Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[Lex] Only warn on defining or undefining language-defined builtins

Authored by john.brawn on May 30 2023, 11:47 AM.



D144654 made it so that we warn on any defining or undefining of builtin macros. However the C and C++ standards only forbid the defining or undefining of macros defined in the language standard itself, but clang defines more macros than those and warning on those may not be helpful.

Resolve this by only warning if the builtin macro name is the name of a macro defined by the language. This is done in a way that removes some of the existing checks, as those were made redundant by restricting the warning in this way.

Diff Detail

Event Timeline

john.brawn created this revision.May 30 2023, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2023, 11:47 AM
john.brawn requested review of this revision.May 30 2023, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2023, 11:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers added inline comments.

Please add tests for the rest of AFAICT, i386 requires -m32 to be defined in the first place.


Should these depend on LangOpt?

This patch cleared up all the warnings I reported, thank you for the fix!

Thanks! +1 from me, this does clear up all the issues I've had - it supersedes D151662 and makes the external patch to avoid doing -U__STRICT_ANSI__ unnecessary.

Add more testing.

john.brawn marked an inline comment as done.May 31 2023, 7:25 AM
john.brawn added inline comments.

I don't think so. If we're e.g. not compiling C++ then __cplusplus won't be defined so it doesn't matter if we check for it.


This line wont have coverage unless the RUN line sets -m32 (even then, an explicit triple would be better for non-x86 hosts running the test).

Maybe add this undef to either init-x86.c or predefined-arch-macros.c?

Move some of the testing to init-x86.c

1728–1741 ↗(On Diff #527164)

Hmm...I guess if this is going to require adding run lines, then let's move these to a new test file in the same dir. Maybe undef-x86.c or whatever? Sorry for back and forth.

john.brawn updated this revision to Diff 527432.Jun 1 2023, 8:35 AM

Put x86 tests in undef-x86.c.

nickdesaulniers accepted this revision.Jun 1 2023, 9:09 AM

Thanks for the patch!


I don't think i686 adds anything beyond i386, at least for this test they are both -m32; consider removing this run line.

This revision is now accepted and ready to land.Jun 1 2023, 9:09 AM
This revision was landed with ongoing or failed builds.Jun 1 2023, 9:38 AM
This revision was automatically updated to reflect the committed changes.