You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/02/02 20:22:35 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
xiaoxiang781216 opened a new pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399
## Summary
Align NDEBUG with CONFIG_DEBUG_ASSERTIONS
## Impact
assert macro
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1030676406
> Where you said Yes.
>
> "If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled **unless I define NDEBUG** in that file or globally."
>
> Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow
>
> I think the difference of opinion comes from the fact that I see the NuttX debugging (DEBUGASSERT and all the debug printing) as separate from assert().
I am strange that the standard define NDEBUG not DEBUG. But the name isn't the critical issue, the key point is the default setting should turn off assert, because NutX generate the release(not debug) image by default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029749098
> @xiaoxiang781216
>
> This still feels wrong to me.
>
> If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled unless I define NDEBUG in that file or globally.
>
Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow.
> [from](https://en.cppreference.com/w/c/error/assert#:~:text=If%20NDEBUG%20is%20defined%20as,the%20source%20code%20where%20%3Cassert.&text=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort().)
>
> ```
> The definition of the macro assert depends on another macro, NDEBUG, which is not defined by the standard library.
>
> If NDEBUG is defined as a macro name at the point in the source code where <assert.h> is included, then assert does nothing.
> ```
>
> Also the inversion and naming is confusing given we have DEBUGASSERT
>
Yes, that's why I don't want to introduce new config but reuse CONFIG_DEBUG_ASSERTIONS. is It practical to distinguish assert and DEBUGASSERT? I doubt it, but I don't want to waste my time to discuss the boring topic. So, you have to accept the confusion regardless how we name the new option.
> DEBUG_ASSERT bool "Enable assert macro" default n
>
> would be better served as "Globally disable assert() macros (define NDEBUG)"
>
> DISABLE_ASSERT bool "Globally disable assert() macros (define NDEBUG)" default n
>
It doesn't matter to me, either is fine to me.
> Yes assertion are enabled by default.
No, I don't agree the default value. assert is for debug only, we need follow NuttX practice: all debug feature need enable explicitly:
https://github.com/apache/incubator-nuttx/blob/master/Kconfig#L558-L1955
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029749098
> @xiaoxiang781216
>
> This still feels wrong to me.
>
> If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled unless I define NDEBUG in that file or globally.
>
Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow.
> [from](https://en.cppreference.com/w/c/error/assert#:~:text=If%20NDEBUG%20is%20defined%20as,the%20source%20code%20where%20%3Cassert.&text=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort().)
>
> ```
> The definition of the macro assert depends on another macro, NDEBUG, which is not defined by the standard library.
>
> If NDEBUG is defined as a macro name at the point in the source code where <assert.h> is included, then assert does nothing.
> ```
>
> Also the inversion and naming is confusing given we have DEBUGASSERT
>
Yes, that's why I don't want to introduce new config but reuse CONFIG_DEBUG_ASSERTIONS. is It practical to distinguish assert and DEBUGASSERT? I doubt it, but I don't want to waste my time to discuss the boring topic. So, you have to accept the confusion regardless how we name the new option.
> DEBUG_ASSERT bool "Enable assert macro" default n
>
> would be better served as "Globally disable assert() macros (define NDEBUG)"
>
> DISABLE_ASSERT bool "Globally disable assert() macros (define NDEBUG)" default n
>
It doesn't matter to me, either is fine to me. But I think many people will still confuse why we has both DISABLE_ASSERT and DEBUG_ASSERTIONS.
> Yes assertion are enabled by default.
No, I don't agree the default value. assert is for debug only, we need follow NuttX practice: all debug feature need enable explicitly:
https://github.com/apache/incubator-nuttx/blob/master/Kconfig#L558-L1955
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1030644415
> @xiaoxiang781216
>
> > > This still feels wrong to me.
>
> > > If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled unless I define NDEBUG in that file or globally.
>
> > Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow.
>
> > Ok, I name the new option to CONFIG_NDEBUG and default to y.
>
> Why did you disable assert by default?
All debug facility should be disabled by default to follow the convention:
https://github.com/apache/incubator-nuttx/blob/master/Kconfig#L558-L1955
You can see all debugging option in above Kconfig snippet is in the off state.
It's very bad that some debugging facility is on , but other is off by default.
If you like all debugging option on by default, please create a new PR to change them.
> That is counter to where you agreed above.
I agree to add an option to decouple assert from DEBUG_ASSERT, can you point out where I agree that the default setting is enable assert?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1031544846
@xiaoxiang781216 yes. Have you considered that @patacongo had a specific intent to having DEGUASSERT separate from assert? I have always assumed there was intent. I may be wrong. But the action to separate them seemed rather deliberate.
In a NuttX "Release" build DEGUASSERT is off (all debug is off to show off the build size). I should still be able to build the app code with assert() and not have to use a Kconfig to enable it.
I can see if one were trying to use some 3rd party code laced with asserts and that one is avoiding changing that code. This may be so important.
I just do not think is should default to off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1030636311
@xiaoxiang781216
>>This still feels wrong to me.
>>If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled unless I define NDEBUG in that file or globally.
>Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow.
> Ok, I name the new option to CONFIG_NDEBUG and default to y.
Why did you disable assert by default? That is counter to where you agreed above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1038601386
@davids5 more people prefer the default value is better to align with other debugging related option. How to avoid the PR stuck for a long time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1039099037
Ok, please raise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1031682222
> @xiaoxiang781216 yes. Have you considered that @patacongo had a specific intent to having DEGUASSERT separate from assert? I have always assumed there was intent. I may be wrong. But the action to separate them seemed rather deliberate.
@patacongo could you give your opinion?
>
> In a NuttX "Release" build DEGUASSERT is off (all debug is off to show off the build size). I should still be able to build the app code with assert() and not have to use a Kconfig to enable it.
>
> I can see if one were trying to use some 3rd party code laced with asserts and that one is avoiding changing that code. This may be so important.
>
You can do what you want without modify the code with single line in your app's Makefile:
```
CFLAGS += -UNDEBUG
```
> I just do not think is should default to off.
You can turn on in your defconfig globally, or in your particular apps.
Since many developers don't have the deep knowledge of individual Kconfig, we need ensure the default config in the production ready state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 merged pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029387606
@xiaoxiang781216
This still feels wrong to me.
If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled unless I define NDEBUG in that file or globally.
[from](https://en.cppreference.com/w/c/error/assert#:~:text=If%20NDEBUG%20is%20defined%20as,the%20source%20code%20where%20%3Cassert.&text=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort().)
```
The definition of the macro assert depends on another macro, NDEBUG, which is not defined by the standard library.
If NDEBUG is defined as a macro name at the point in the source code where <assert.h> is included, then assert does nothing.
```
Also the inversion and naming is confusing given we have DEBUGASSERT
DEBUG_ASSERT
bool "Enable assert macro"
default n
would be better served as "Globally disable assert() macros (define NDEBUG)"
DISABLE_ASSERT
bool "Globally disable assert() macros (define NDEBUG)"
default n
Yes assertion are enabled by default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029169962
@xiaoxiang781216 I agree with @yamt this does not seem like it should be coupled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1030677919
@davids5 if you are familiar with any modem IDE development environment(e.g. Visual Studio, Eclipse):
https://stackoverflow.com/questions/933739/what-is-the-difference-between-release-and-debug-modes-in-visual-studio
The project file generated by IDE normally has two build mode(Debug and Release) at the same time, the major difference is whether NDEBUG is defined and which version of library(debug v.s. release) from toolchain will be linked.
Compare with IDE, you can think that NuttX can only provide one build mode at the same time, and the build mode is release by default. If you want to switch debug mode, you have to enable(kconfig-tweak) the related option in the same config, or create a new defconfig for debug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1028421929
i feel this is too automatic.
CONFIG_DEBUG_ASSERTIONS is about NuttX specific DEBUGASSERT.
NDEBUG is in the standards and about assert.
it's usually a per-app thing.
some apps might be even broken with NDEBUG.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1028581598
> i feel this is too automatic.
>
> CONFIG_DEBUG_ASSERTIONS is about NuttX specific DEBUGASSERT.
>
Why do you think it isn't a good idea to make assert same as DEBUGASSERT by default? But user can still modify the behavior by CFLAGS += -DNDEBUG or CLFAGS += -UNDEBUG in it's Makefile?
> NDEBUG is in the standards and about assert. it's usually a per-app thing. some apps might be even broken with NDEBUG.
app could overwrite it by CFLAGS += -UNDEBUG.
> i want to be able to control DEBUGASSERT and assert separately.
so you want to add a new Kconfig?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1028581598
> i feel this is too automatic.
>
> CONFIG_DEBUG_ASSERTIONS is about NuttX specific DEBUGASSERT.
>
Why do you think it isn't a good idea to make assert same as DEBUGASSERT by default if user can still modify the behavior by CFLAGS += -DNDEBUG or CLFAGS += -UNDEBUG?
> NDEBUG is in the standards and about assert. it's usually a per-app thing. some apps might be even broken with NDEBUG.
app could overwrite it by CFLAGS += -UNDEBUG.
> i want to be able to control DEBUGASSERT and assert separately.
so you want to add a new Kconfig?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029762886
> It doesn't matter to me, either is fine to me. But I think many people will still confuse why we has both DISABLE_ASSERT(disable assertion) and DEBUG_ASSERTIONS(enable assertion) at the same time.
>
Ok, I name the new option to CONFIG_NDEBUG and default to y.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1038672755
fwiw, i slightly prefer NDEBUG=n as the default. just because it's more conservative.
but i can understand the opposite opinion.
i don't care much as far as it's separated from DEBUGASSERT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1030644415
> @xiaoxiang781216
>
> > > This still feels wrong to me.
>
> > > If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled unless I define NDEBUG in that file or globally.
>
> > Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow.
>
> > Ok, I name the new option to CONFIG_NDEBUG and default to y.
>
> Why did you disable assert by default?
By convention, debug facility should be disabled by default:
https://github.com/apache/incubator-nuttx/blob/master/Kconfig#L558-L1955
You can see all debugging option in above Kconfig snippet is in the off state.
It's very bad that some debugging facility is on , but other is off by default.
If you like all debugging option on by default, please create a new PR to change them.
> That is counter to where you agreed above.
I agree to add an option to decouple assert from DEBUG_ASSERT, can you point out where I agree that the default setting is enable assert?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1041911741
@davids5 look like nobody care about this, how to proceed in the next?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1028612384
> > > i feel this is too automatic.
> > > CONFIG_DEBUG_ASSERTIONS is about NuttX specific DEBUGASSERT.
> >
> >
> > Why do you think it isn't a good idea to make assert same as DEBUGASSERT by default? But user can still modify the behavior by CFLAGS += -DNDEBUG or CLFAGS += -UNDEBUG in it's Makefile?
>
> it might be a good idea in the ideal world. but i don't want to audit all app code for this change.
>
> > > NDEBUG is in the standards and about assert. it's usually a per-app thing. some apps might be even broken with NDEBUG.
In this case, you can disable CONFIG_DEBUG_ASSERTIONS.
> >
> >
> > app could overwrite it by CFLAGS += -UNDEBUG.
> > > i want to be able to control DEBUGASSERT and assert separately.
> >
> >
> > so you want to add a new Kconfig?
>
> i have no problem if it was a separate Kconfig.
Ok, I can add one, but the default is still NDEBUG.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1028603008
> > i feel this is too automatic.
> > CONFIG_DEBUG_ASSERTIONS is about NuttX specific DEBUGASSERT.
>
> Why do you think it isn't a good idea to make assert same as DEBUGASSERT by default? But user can still modify the behavior by CFLAGS += -DNDEBUG or CLFAGS += -UNDEBUG in it's Makefile?
it might be a good idea in the ideal world.
but i don't want to audit all app code for this change.
>
> > NDEBUG is in the standards and about assert. it's usually a per-app thing. some apps might be even broken with NDEBUG.
>
> app could overwrite it by CFLAGS += -UNDEBUG.
>
> > i want to be able to control DEBUGASSERT and assert separately.
>
> so you want to add a new Kconfig?
i have no problem if it was a separate Kconfig.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029749098
> @xiaoxiang781216
>
> This still feels wrong to me.
>
> If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled unless I define NDEBUG in that file or globally.
>
Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow.
> [from](https://en.cppreference.com/w/c/error/assert#:~:text=If%20NDEBUG%20is%20defined%20as,the%20source%20code%20where%20%3Cassert.&text=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort().)
>
> ```
> The definition of the macro assert depends on another macro, NDEBUG, which is not defined by the standard library.
>
> If NDEBUG is defined as a macro name at the point in the source code where <assert.h> is included, then assert does nothing.
> ```
>
> Also the inversion and naming is confusing given we have DEBUGASSERT
>
Yes, that's why I don't want to introduce new config but reuse CONFIG_DEBUG_ASSERTIONS. is It practical to distinguish assert and DEBUGASSERT? I doubt it, but I don't want to waste my time to discuss the boring topic. So, you have to accept the confusion regardless how we name the new option.
> DEBUG_ASSERT bool "Enable assert macro" default n
>
> would be better served as "Globally disable assert() macros (define NDEBUG)"
>
> DISABLE_ASSERT bool "Globally disable assert() macros (define NDEBUG)" default n
>
It doesn't matter to me, either is fine to me. But I think many people will still confuse why we has both DISABLE_ASSERT(disable assertion) and DEBUG_ASSERTIONS(enable assertion) at the same time.
> Yes assertion are enabled by default.
No, I don't agree the default value. assert is for debug only, we need follow NuttX practice: all debug feature need enable explicitly:
https://github.com/apache/incubator-nuttx/blob/master/Kconfig#L558-L1955
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: tools/Config.mk: Define NDEBUG if CONFIG_DEBUG_ASSERTIONS isn't defined
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029355840
Done, @davids5 @yamt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1030644415
> > Ok, I name the new option to CONFIG_NDEBUG and default to y.
>
> Why did you disable assert by default?
By convention, debug facility should be disabled by default:
https://github.com/apache/incubator-nuttx/blob/master/Kconfig#L558-L1955
You can see all debugging option in above Kconfig snippet is in the off state.
It's very bad that some debugging facility is on , but other is off by default.
If you like all debugging option on by default, please create a new PR to change them.
> That is counter to where you agreed above.
I agree to add an option to decouple assert from DEBUG_ASSERT, can you point out where I agree that the default setting is enable assert?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1031046658
@davids5 do you have more concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1030672301
Where you said Yes.
"If I place "assert(...)" in a file I expect not have to also enabled it, but that it will be enabled **unless I define NDEBUG** in that file or globally."
Yes, this behavior is strange to me(that's why I provide this patch), but it is defined by standard and then we have to follow
I think the difference of opinion comes from the fact that I see the NuttX debugging (DEBUGASSERT and all the debug printing) as separate from assert().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #5399: Add CONFIG_NDEBUG Kconfig to control NDEBUG definition
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5399:
URL: https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1039078862
@xiaoxiang781216 - Call a vote?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org