You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "Gary-Hobson (via GitHub)" <gi...@apache.org> on 2024/10/10 06:09:12 UTC
[PR] include/strings: replace ffs from an inline function to a macro [nuttx]
Gary-Hobson opened a new pull request, #14028:
URL: https://github.com/apache/nuttx/pull/14028
## Summary
using static inline functions sometimes generates compilation warnings: nxfonts/nxfonts_bitmaps_sans23x27.c:168:50: warning: declaration of ‘ffs’ shadows a built-in function [-Wshadow]
168 | static __attribute__((always_inline)) inline int ffs(int j)
This problem also seems to exist in the Linux kernel: https://lore.kernel.org/all/20220812114438.1574-2-mailhol.vincent@wanadoo.fr/
## Impact
## Testing
--
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
Re: [PR] include/strings: replace ffs from an inline function to a macro [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #14028:
URL: https://github.com/apache/nuttx/pull/14028#discussion_r1794792486
##########
include/strings.h:
##########
@@ -73,73 +73,46 @@ extern "C"
****************************************************************************/
#ifdef CONFIG_HAVE_BUILTIN_FFS
-static inline_function int ffs(int j)
-{
- return __builtin_ffs(j);
-}
+#define ffs(j) __builtin_ffs(j)
#elif defined (CONFIG_HAVE_BUILTIN_CTZ)
-static inline_function int ffs(int j)
-{
- return __builtin_ctz(j) + 1;
-}
+# define ffs(j) (__builtin_ctz(j) + 1)
#else
int ffs(int j);
#endif
#ifdef CONFIG_HAVE_BUILTIN_FFSL
-static inline_function int ffsl(long j)
-{
- return __builtin_ffsl(j);
-}
-#elif defined (CONFIG_HAVE_BUILTIN_CTZ)
-static inline_function int ffsl(long j)
-{
- return __builtin_ctzl(j) + 1;
-}
+#define ffsl(j) __builtin_ffsl(j)
+#elif defined (CONFIG_HAVE_BUILTIN_CTZL)
+# define ffsl(j) (__builtin_ctzl(j) + 1)
#else
int ffsl(long j);
#endif
#ifdef CONFIG_HAVE_LONG_LONG
# ifdef CONFIG_HAVE_BUILTIN_FFSLL
-static inline_function int ffsll(long long j)
-{
- return __builtin_ffsll(j);
-}
-# elif defined (CONFIG_HAVE_BUILTIN_CTZ)
-static inline_function int ffsll(long long j)
-{
- return __builtin_ctzll(j) + 1;
-}
+# define ffsll(j) __builtin_ffsll(j)
+# elif defined (CONFIG_HAVE_BUILTIN_CTZLL)
+# define ffsll(j) (__builtin_ctzll(j) + 1)
# else
int ffsll(long long j);
# endif
#endif
#ifdef CONFIG_HAVE_BUILTIN_CLZ
-static inline_function int fls(int j)
-{
- return (8 * sizeof(int)) - __builtin_clz(j);
-}
+# define fls(j) ((8 * sizeof(int)) - __builtin_clz(j))
#else
int fls(int j);
#endif
-#ifdef CONFIG_HAVE_BUILTIN_CLZ
-static inline_function int flsl(long j)
-{
- return (8 * sizeof(long)) - __builtin_clzl(j);
-}
+#ifdef CONFIG_HAVE_BUILTIN_CLSL
+# define flsl(j) ((8 * sizeof(long)) - __builtin_clsl(j))
#else
int flsl(long j);
#endif
#ifdef CONFIG_HAVE_LONG_LONG
-# ifdef CONFIG_HAVE_BUILTIN_CLZ
-static inline_function int flsll(long long j)
-{
- return (8 * sizeof(long long)) - __builtin_clzll(j);
-}
+# ifdef CONFIG_HAVE_BUILTIN_CLZLL
+# define flsll(j) ((8 * sizeof(long long)) - __builtin_clzll(j))
Review Comment:
```suggestion
# define flsll(j) ((8 * sizeof(long long)) - __builtin_clzll(j))
```
##########
include/strings.h:
##########
@@ -73,73 +73,46 @@ extern "C"
****************************************************************************/
#ifdef CONFIG_HAVE_BUILTIN_FFS
-static inline_function int ffs(int j)
-{
- return __builtin_ffs(j);
-}
+#define ffs(j) __builtin_ffs(j)
#elif defined (CONFIG_HAVE_BUILTIN_CTZ)
-static inline_function int ffs(int j)
-{
- return __builtin_ctz(j) + 1;
-}
+# define ffs(j) (__builtin_ctz(j) + 1)
#else
int ffs(int j);
#endif
#ifdef CONFIG_HAVE_BUILTIN_FFSL
-static inline_function int ffsl(long j)
-{
- return __builtin_ffsl(j);
-}
-#elif defined (CONFIG_HAVE_BUILTIN_CTZ)
-static inline_function int ffsl(long j)
-{
- return __builtin_ctzl(j) + 1;
-}
+#define ffsl(j) __builtin_ffsl(j)
Review Comment:
ditto
##########
include/strings.h:
##########
@@ -73,73 +73,46 @@ extern "C"
****************************************************************************/
#ifdef CONFIG_HAVE_BUILTIN_FFS
-static inline_function int ffs(int j)
-{
- return __builtin_ffs(j);
-}
+#define ffs(j) __builtin_ffs(j)
Review Comment:
indent
--
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
Re: [PR] include/strings: replace ffs from an inline function to a macro [nuttx]
Posted by "Gary-Hobson (via GitHub)" <gi...@apache.org>.
Gary-Hobson closed pull request #14028: include/strings: replace ffs from an inline function to a macro
URL: https://github.com/apache/nuttx/pull/14028
--
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
Re: [PR] include/strings: replace ffs from an inline function to a macro [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #14028:
URL: https://github.com/apache/nuttx/pull/14028#issuecomment-2404141051
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
The PR summary provides context, explains the issue (shadowing built-in functions), and references an external example. However, it lacks crucial information:
* **Missing: What functional part is changed?** Which files are modified to address the warning? Is it a simple rename or a larger refactoring?
* **Missing: How the change works.** Are `static inline` declarations changed? Alternatives used?
* **Impact is entirely empty.** This is critical for reviewers.
**Testing is insufficient:**
* **No specifics on build hosts or targets.** Saying "CI" isn't helpful without knowing the CI configurations. OS, compiler versions, target architectures are essential.
* **Logs are placeholders.** Real logs demonstrating the problem *before* and the fix *after* are mandatory.
**Verdict:** **This PR does NOT meet NuttX requirements.** It needs significant improvements to be considered:
1. **Complete the Summary** with specifics on code changes and how they address the issue.
2. **Fill out the Impact section thoroughly.** Consider ALL points, even if the answer is "NO" (briefly explain why).
3. **Provide REAL testing logs** showing the warning/error before and its absence after the change. Be specific about your testing environment.
--
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
Re: [PR] include/strings: replace ffs from an inline function to a macro [nuttx]
Posted by "Gary-Hobson (via GitHub)" <gi...@apache.org>.
Gary-Hobson commented on code in PR #14028:
URL: https://github.com/apache/nuttx/pull/14028#discussion_r1794887819
##########
include/strings.h:
##########
@@ -73,73 +73,46 @@ extern "C"
****************************************************************************/
#ifdef CONFIG_HAVE_BUILTIN_FFS
-static inline_function int ffs(int j)
-{
- return __builtin_ffs(j);
-}
+#define ffs(j) __builtin_ffs(j)
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