Fix: Limit scope of macro redefinitions in _ctype.h #366
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The macros `static` and `__inline` were being redefined to empty when `_EXTERNALIZE_CTYPE_INLINES_` was defined. However, these redefinitions were not scoped, leading to potential conflicts with other headers or your code that relies on the standard behavior of these keywords. This change ensures that `static` and `__inline` are undefined (using `#undef`) immediately after the block of code where their redefinition is necessary for generating externalized function prototypes. This restores their standard keyword meaning for any subsequent code, preventing unintended side-effects and improving standards compliance. A targeted verification was created to confirm that the macros are correctly undefined and that standard `static` and `__inline` usage is unaffected after the modified block.
|
Can one of the admins verify this patch? |
Reviewer's GuideThis PR refines the scoping of macro redefinitions in _ctype.h by undefining static and __inline post-inline block, standardizes header includes and type definitions across key headers, updates the endian.h inclusion, and adds targeted verification tests to ensure no regressions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The macros `static` and `__inline` were being redefined to empty when `_EXTERNALIZE_CTYPE_INLINES_` was defined. However, these redefinitions were not scoped, leading to potential conflicts with other headers or your code that relies on the standard behavior of these keywords. This change ensures that `static` and `__inline` are undefined (using `#undef`) immediately after the block of code where their redefinition is necessary for generating externalized function prototypes. This restores their standard keyword meaning for any subsequent code, preventing unintended side-effects and improving standards compliance. A targeted verification was created to confirm that the macros are correctly undefined and that standard `static` and `__inline` usage is unaffected after the modified block.
|
@sourcery-ai review
I made the necessary corrections.
12 Haz 2025 Per 12:42 tarihinde sourcery-ai[bot] ***@***.***>
şunu yazdı:
… *sourcery-ai[bot]* left a comment (ghostbsd/ghostbsd-src#366)
<#366 (comment)>
🧙 Sourcery is reviewing your pull request!
------------------------------
Tips and commands Interacting with Sourcery
- *Trigger a new review:* Comment @sourcery-ai review on the pull
request.
- *Continue discussions:* Reply directly to Sourcery's review comments.
- *Generate a GitHub issue from a review comment:* Ask Sourcery to
create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
- *Generate a pull request title:* Write @sourcery-ai anywhere in the
pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at
any time.
- *Generate a pull request summary:* Write @sourcery-ai summary
anywhere in
the pull request body to generate a PR summary at any time exactly
where you
want it. You can also comment @sourcery-ai summary on the pull request
to
(re-)generate the summary at any time.
- *Generate reviewer's guide:* Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
- *Resolve all Sourcery comments:* Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
- *Dismiss all Sourcery reviews:* Comment @sourcery-ai dismiss on the
pull
request to dismiss all existing Sourcery reviews. Especially useful if
you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Customizing Your Experience
Access your dashboard <https://app.sourcery.ai> to:
- Enable or disable review features such as the Sourcery-generated
pull request
summary, the reviewer's guide, and others.
- Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team ***@***.***> for questions or
feedback.
- Visit our documentation <https://docs.sourcery.ai> for detailed
guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter
<https://x.com/SourceryAI>, LinkedIn
<https://www.linkedin.com/company/sourcery-ai/> or GitHub
<https://github.com/sourcery-ai>.
—
Reply to this email directly, view it on GitHub
<#366 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOMX7Z6W73RPQI4VUHMX7HT3DFDRDAVCNFSM6AAAAAB7E4RCPKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNRVHEYDCMJXHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rkarahan80 - I've reviewed your changes - here's some feedback:
- Rather than manually #undef-ing
staticand__inline, consider using#pragma push_macro/pop_macroto scope and restore the original definitions in a more robust, maintainable way. - The fallback typedefs for
__rune_tand__size_tmay not match platform ABI types—consider pulling these from existing system headers or adding static assertions to ensure they’re consistent with the real definitions. - Your custom include-guard macros (
__RUNE_T_DEFINED_IN_RUNETYPE_H, etc.) could collide with other headers—align them with the platform’s naming convention or namespace them more uniquely to avoid accidental redefinition.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| /* Define __ct_rune_t based on __rune_t, which should be defined via stddef.h->sys/types.h */ | ||
| /* stddef.h is included via sys/types.h which is included from ctype.h/_ctype.h */ | ||
| #if __BSD_VISIBLE /* Matches guard in stddef.h for rune_t */ |
There was a problem hiding this comment.
suggestion (bug_risk): Guard feature macro with defined() check
Use '#if defined(__BSD_VISIBLE) && __BSD_VISIBLE' to avoid preprocessor errors if __BSD_VISIBLE is not defined.
Suggested change
| #if __BSD_VISIBLE /* Matches guard in stddef.h for rune_t */ | |
| #if defined(__BSD_VISIBLE) && __BSD_VISIBLE /* Matches guard in stddef.h for rune_t */ |
Comment on lines
41
to
44
| #ifndef _PTRDIFF_T_DECLARED | ||
| typedef __ptrdiff_t ptrdiff_t; | ||
| typedef __PTRDIFF_TYPE__ ptrdiff_t; | ||
| #define _PTRDIFF_T_DECLARED | ||
| #endif |
There was a problem hiding this comment.
suggestion: Using PTRDIFF_TYPE is compiler-specific
Add a guard or fallback for compilers that do not support PTRDIFF_TYPE, such as using __ptrdiff_t as an alternative.
Suggested change
| #ifndef _PTRDIFF_T_DECLARED | |
| typedef __ptrdiff_t ptrdiff_t; | |
| typedef __PTRDIFF_TYPE__ ptrdiff_t; | |
| #define _PTRDIFF_T_DECLARED | |
| #endif | |
| #ifndef _PTRDIFF_T_DECLARED | |
| #if defined(__PTRDIFF_TYPE__) | |
| typedef __PTRDIFF_TYPE__ ptrdiff_t; | |
| #else | |
| typedef __ptrdiff_t ptrdiff_t; | |
| #endif | |
| #define _PTRDIFF_T_DECLARED | |
| #endif |
Comment on lines
+52
to
+53
| // rA3 (my_inline_func_A) and rB1 (normal_inline_func_B) are harder to test for "inlineness" | ||
| // without inspecting assembly, but compilation success is key. |
There was a problem hiding this comment.
issue (testing): Assert return values for my_inline_func_A and normal_inline_func_B.
Please add assertions for rA3 and rB1 to verify their return values, as is done for rA1 and rA2. This will help ensure the functions return the expected results.
|
Unfortunately, I don't have time to make the necessary arrangements. You
can close it if you want. Thank you.
12 Haz 2025 Per 12:47 tarihinde sourcery-ai[bot] ***@***.***>
şunu yazdı:
… ***@***.***[bot]* commented on this pull request.
Hey @rkarahan80 <https://github.com/rkarahan80> - I've reviewed your
changes - here's some feedback:
- Rather than manually #undef-ing static and __inline, consider using #pragma
push_macro/pop_macro to scope and restore the original definitions in
a more robust, maintainable way.
- The fallback typedefs for __rune_t and __size_t may not match
platform ABI types—consider pulling these from existing system headers or
adding static assertions to ensure they’re consistent with the real
definitions.
- Your custom include-guard macros (__RUNE_T_DEFINED_IN_RUNETYPE_H,
etc.) could collide with other headers—align them with the platform’s
naming convention or namespace them more uniquely to avoid accidental
redefinition.
Here's what I looked at during the review
- 🟡 *General issues*: 2 issues found
- 🟢 *Security*: all looks good
- 🟡 *Testing*: 1 issue found
- 🟢 *Documentation*: all looks good
------------------------------
Sourcery is free for open source - if you like our reviews please consider
sharing them ✨
- X
<https://twitter.com/intent/tweet?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai>
- Mastodon
<https://mastodon.social/share?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai>
- LinkedIn
<https://www.linkedin.com/sharing/share-offsite/?url=https://sourcery.ai>
- Facebook
<https://www.facebook.com/sharer/sharer.php?u=https://sourcery.ai>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use
the feedback to improve your reviews.
------------------------------
In include/_ctype.h
<#366 (comment)>
:
> @@ -44,7 +44,17 @@
#define __CTYPE_H_
#include <sys/cdefs.h>
-#include <sys/_types.h>
+#include <sys/types.h>
+#include <runetype.h> /* For __rune_t */
+
+/* Define __ct_rune_t based on __rune_t, which should be defined via stddef.h->sys/types.h */
+/* stddef.h is included via sys/types.h which is included from ctype.h/_ctype.h */
+#if __BSD_VISIBLE /* Matches guard in stddef.h for rune_t */
*suggestion (bug_risk):* Guard feature macro with defined() check
Use '#if defined(__BSD_VISIBLE) && __BSD_VISIBLE' to avoid preprocessor
errors if __BSD_VISIBLE is not defined.
⬇️ Suggested change
-#if __BSD_VISIBLE /* Matches guard in stddef.h for rune_t */
+#if defined(__BSD_VISIBLE) && __BSD_VISIBLE /* Matches guard in stddef.h for rune_t */
------------------------------
In include/stddef.h
<#366 (comment)>
:
> #ifndef _PTRDIFF_T_DECLARED
-typedef __ptrdiff_t ptrdiff_t;
+typedef __PTRDIFF_TYPE__ ptrdiff_t;
#define _PTRDIFF_T_DECLARED
#endif
*suggestion:* Using *PTRDIFF_TYPE* is compiler-specific
Add a guard or fallback for compilers that do not support *PTRDIFF_TYPE*,
such as using __ptrdiff_t as an alternative.
⬇️ Suggested change
-#ifndef _PTRDIFF_T_DECLARED
-typedef __ptrdiff_t ptrdiff_t;
-typedef __PTRDIFF_TYPE__ ptrdiff_t;
-#define _PTRDIFF_T_DECLARED
-#endif
+#ifndef _PTRDIFF_T_DECLARED
+#if defined(__PTRDIFF_TYPE__)
+typedef __PTRDIFF_TYPE__ ptrdiff_t;
+#else
+typedef __ptrdiff_t ptrdiff_t;
+#endif
+#define _PTRDIFF_T_DECLARED
+#endif
------------------------------
In macro_verification_test.c
<#366 (comment)>
:
> + // rA3 (my_inline_func_A) and rB1 (normal_inline_func_B) are harder to test for "inlineness"
+ // without inspecting assembly, but compilation success is key.
*issue (testing):* Assert return values for my_inline_func_A and
normal_inline_func_B.
Please add assertions for rA3 and rB1 to verify their return values, as
is done for rA1 and rA2. This will help ensure the functions return the
expected results.
—
Reply to this email directly, view it on GitHub
<#366 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOMX7Z3Q2UZCYLWPLSMXOPT3DFEBXAVCNFSM6AAAAAB7E4RCPKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSMRQGMZTQMJUGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I understand your changes, but this should be done upstream on FreeBSD's FreeBSD-src. |
|
I forked the original Github page. I will be using the original page from
now on.
12 Haz 2025 Per 12:54 tarihinde xcrsz ***@***.***> şunu yazdı:
… *xcrsz* left a comment (ghostbsd/ghostbsd-src#366)
<#366 (comment)>
I understand your changes, but this should be done upstream on FreeBSD's
FreeBSD-src.
—
Reply to this email directly, view it on GitHub
<#366 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOMX7Z3HWB62XUAZTAVTNG33DFE5LAVCNFSM6AAAAAB7E4RCPKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNRVHE2DGNZQGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The macros
staticand__inlinewere being redefined to empty when_EXTERNALIZE_CTYPE_INLINES_was defined. However, these redefinitions were not scoped, leading to potential conflicts with other headers or your code that relies on the standard behavior of these keywords.This change ensures that
staticand__inlineare undefined (using#undef) immediately after the block of code where their redefinition is necessary for generating externalized function prototypes. This restores their standard keyword meaning for any subsequent code, preventing unintended side-effects and improving standards compliance.A targeted verification was created to confirm that the macros are correctly undefined and that standard
staticand__inlineusage is unaffected after the modified block.Summary by Sourcery
Ensure that
staticand__inlinemacros in_ctype.hare strictly scoped and undefined after use to prevent keyword conflicts; replace direct imports of internal type headers with<sys/types.h>and guarded compiler-based typedefs for__rune_t,__size_t,__off_t, and__va_list; updatestddef.hto leverage compiler intrinsic type macros; remove__pureattributes from internal ctype function declarations; swap<sys/_endian.h>for<endian.h>; and add targeted tests to verify macro cleanup and standard keyword behavior.Bug Fixes:
staticand__inlineredefinitions in_ctype.hto restore their standard meanings.Enhancements:
<sys/_types.h>and<sys/_null.h>with<sys/types.h>and introduce guarded typedefs for internal types (__rune_t,__size_t,__off_t,__va_list).stddef.hto use compiler intrinsic type macros (__PTRDIFF_TYPE__,__SIZE_TYPE__,__WCHAR_TYPE__).__pureattributes from internal___runetype,___tolower, and___toupperdeclarations.<sys/_endian.h>to<endian.h>.Tests:
macro_verification_test.candverification_test.cto confirm macro undefinition behavior and ensure normalstatic/__inlineandctype.hfunctionality.