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/28 20:03:23 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
xiaoxiang781216 opened a new pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647
## Summary
Improve the compatibility of assert
## Impact
Minor change
## 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] pkarashchenko merged pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055179969
But `assert.h` includes `#include <nuttx/compiler.h>` and `UNUSED` is defined in `compiler.h` so the redefinition problem will still exists. I mean the problem will not be solved globally.
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055299126
Yeah, but that is include order dependant.
We can merge this change if that solves your issue, but in general I'm against some partial solutions.
--
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 #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055206321
> But `assert.h` includes `#include <nuttx/compiler.h>` and `UNUSED` is defined in `compiler.h` so the redefinition problem will still exists. I mean the problem will not be solved globally.
the third party code write like this:
```
#undef UNUSED
#define UNUSED __attribute__((unused))
```
> It's a chicken-egg problem. We meet similar with OpenSBI integration. Really do not know what is the best solution.
Yes, but this simple change could fix the problem we meet. It could reduce the possibility in the future.
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1056508269
> > @xiaoxiang781216 I think that we can drop dependency on `nuttx/compiler.h` now.
>
> I think so before, but assert reference noreturn_function:(.
:( I missed that.
--
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 #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055176162
> I see that `# define UNUSED(a) ((void)(1 || (a)))`, so why this change is needed?
UNUSED is frequently redefined by many third party to something like this:
```
# define UNUSED __attribute__((unused))
```
Since assert(assert.h) is the most used macro, it's better to remove the dependence on UNUSED macro.
--
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 #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1056502008
> @xiaoxiang781216 I think that we can drop dependency on `nuttx/compiler.h` now.
I think so before, but assert reference noreturn_function:(.
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055188698
Maybe better to rename `UNUSED` to `__UNUSED` or something like this?
--
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 #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055694034
> The another option is to add `#undef UNUSED` at the beginning of `nuttx/compiler.h`
It only work when we include the third party header file first and then assert.h, but the order is reverse in our case, that's why I create this patch. Since UNUSED is normally used inside source file. assert.h is the only exception by a recent change:
https://github.com/apache/incubator-nuttx/commit/5f3a98b5a8d39a7a9ee1823c6f7eaf05b1822ad7
It's always good to avoid reference UNUSED in header files.
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055653371
The another option is to add `#undef UNUSED` at the beginning of `nuttx/compiler.h`
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1056484275
@xiaoxiang781216 I think that we can drop dependency on `nuttx/compiler.h` now.
--
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 a change in pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#discussion_r816568818
##########
File path: include/assert.h
##########
@@ -67,7 +67,7 @@
*/
#ifdef NDEBUG
-# define assert(f) UNUSED(f)
+# define assert(f) ((void)(1 || (f)))
Review comment:
Done.
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055173225
I see that `# define UNUSED(a) ((void)(1 || (a)))`, so why this change is needed?
--
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 #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055206321
> But `assert.h` includes `#include <nuttx/compiler.h>` and `UNUSED` is defined in `compiler.h` so the redefinition problem will still exists. I mean the problem will not be solved globally.
the third party code write like this:
```
#undef UNUSED
#define UNUSED __attribute__((unused))
```
--
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] pkarashchenko commented on pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055191249
It's a chicken-egg problem. We meet similar with OpenSBI integration. Really do not know what is the best solution.
--
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] pkarashchenko commented on a change in pull request #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#discussion_r816561776
##########
File path: include/assert.h
##########
@@ -67,7 +67,7 @@
*/
#ifdef NDEBUG
-# define assert(f) UNUSED(f)
+# define assert(f) ((void)(1 || (f)))
Review comment:
```suggestion
# define assert(f) ((void)(1 || (f)))
```
--
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 #5647: assert.h: Don't use UNUSED macro since it's very easy happen conflict
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5647:
URL: https://github.com/apache/incubator-nuttx/pull/5647#issuecomment-1055206321
> But `assert.h` includes `#include <nuttx/compiler.h>` and `UNUSED` is defined in `compiler.h` so the redefinition problem will still exists. I mean the problem will not be solved globally.
the third party code write like this:
```
#undef UNUSED
#define UNUSED __attribute__((unused))
```
> It's a chicken-egg problem. We meet similar with OpenSBI integration. Really do not know what is the best solution.
Yes, but this simple change could fix the problem we meet.
--
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