This patch resolves https://github.com/llvm/llvm-project/issues/55306.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thank you for working on this!
clang/include/clang/Basic/DiagnosticLexKinds.td | ||
---|---|---|
696–698 | I think we need two diagnostics, one for C2x and one for C++2b (https://wg21.link/p2334 was adopted for C++23). Each of these diagnostics should come in a pair: def warn_cxx20_compat_pp_directive : Warning< "use of a '#%select{elifdef|elifndef}0' directive is incompatible with C++ standards before C++2b", InGroup<CXXPre2bCompat>, DefaultIgnore; def ext_cxx20_pp_directive : ExtWarn< "use of a '#%select{elifdef|elifndef}0' directive is a C++2b extension", InGroup<CXX2b>; and similar for C, except with wording about C standards and in the C warning groups. | |
clang/lib/Lex/PPDirectives.cpp | ||
3273–3274 | And then here, you would do something like: unsigned DiagID; if (getLangOpts().CPlusPlus) DiagID = getLangOpts().CPlusPlus2b ? diag::warn_cxx20_compat_pp_directive : diag::ext_cxx20_pp_directive; else DiagID = getLangOpts().C2x ? diag::warn_c17_compat_pp_directive : diag::ext_c2b_pp_directive; Diag(ElifToken, DiagID) << ...; | |
clang/test/Preprocessor/ext-c2x-pp-directive.c | ||
14 ↗ | (On Diff #427898) | Might as well add a declaration so that we don't have to expect the diagnostic. Also, this will need tests for both forms of the diagnostics, C++ tests, and it'd probably be good to verify the behavior when the previous conditional is skipped. e.g., #if 0 #elifdef WHATEVER // We should still warn on this; we wouldn't if we didn't recognize it as a conditional #endif |
Thank you so much for your review!
clang/include/clang/Basic/DiagnosticLexKinds.td | ||
---|---|---|
696–698 | I thought I had to use Extension here, but what is the difference between Warning, ExtWarn, and Extension? |
It looks like precommit CI caught some related failures:
Failed Tests (7): Clang :: Lexer/deprecate-macro.c Clang :: Lexer/unsafe-macro.c Clang :: Preprocessor/elifdef.c Clang :: Preprocessor/if_warning.c Clang :: Preprocessor/ifdef-recover.c Clang :: Preprocessor/macro_misc.c Clang :: Preprocessor/macro_vaopt_check.cpp
clang/include/clang/Basic/DiagnosticLexKinds.td | ||
---|---|---|
696–698 | Great question! Extension diagnostics are warnings that are enabled via -pedantic but otherwise off by default. You'll typically use Extension or ExtWarn for things that are extensions of some sort, but which one you use depends on how important the warning is. We've traditionally handled "use of whatever is a C++NN extension" warnings as ExtWarn whereas the "use of whatever is incompatible with C++ standards before C++NN" warnings are usually Extension. |
Ahhh, my bad. Thank you for your heads up!
clang/include/clang/Basic/DiagnosticLexKinds.td | ||
---|---|---|
696–698 | In this case, compat_pp_directive for C++2b & C2x uses Warning, but why did you choose it rather than Extension? |
clang/include/clang/Basic/DiagnosticLexKinds.td | ||
---|---|---|
696–698 | Oh shoot, I misspoke and said ExtWarn when I meant Warning that's default ignored. Sorry about that, and thank you for asking for clarification! I picked that because it's the pattern used by many other diagnostic pairs: def ext_inline_variable : ExtWarn< "inline variables are a C++17 extension">, InGroup<CXX17>; def warn_cxx14_compat_inline_variable : Warning< "inline variables are incompatible with C++ standards before C++17">, DefaultIgnore, InGroup<CXXPre17Compat>; def ext_for_range_begin_end_types_differ : ExtWarn< "'begin' and 'end' returning different types (%0 and %1) is a C++17 extension">, InGroup<CXX17>; def warn_for_range_begin_end_types_differ : Warning< "'begin' and 'end' returning different types (%0 and %1) is incompatible " "with C++ standards before C++17">, InGroup<CXXPre17Compat>, DefaultIgnore; def ext_constexpr_body_invalid_stmt : ExtWarn< "use of this statement in a constexpr %select{function|constructor}0 " "is a C++14 extension">, InGroup<CXX14>; def warn_cxx11_compat_constexpr_body_invalid_stmt : Warning< "use of this statement in a constexpr %select{function|constructor}0 " "is incompatible with C++ standards before C++14">, InGroup<CXXPre14Compat>, DefaultIgnore; def ext_constexpr_function_try_block_cxx20 : ExtWarn< "function try block in constexpr %select{function|constructor}0 is " "a C++20 extension">, InGroup<CXX20>; def warn_cxx17_compat_constexpr_function_try_block : Warning< "function try block in constexpr %select{function|constructor}0 is " "incompatible with C++ standards before C++20">, InGroup<CXXPre20Compat>, DefaultIgnore; |
clang/include/clang/Basic/DiagnosticLexKinds.td | ||
---|---|---|
696–698 | Oh, that makes sense. Thank you for your explanation! |
clang/include/clang/Basic/DiagnosticLexKinds.td | ||
---|---|---|
698 | (You may also need to rewrap to 80 col limits.) And do the same for the other three, so that you can use PPElifDiag for it. | |
clang/lib/Lex/PPDirectives.cpp | ||
663 | I like the way you're thinking about this, but I don't like how clever the code is. I'd rather update the comment on PPElifDiag to mention warn_*_compat_pp_directive and ext_*_pp_directive, and then update those diagnostics to have a bogus select slot for the PED_Elif that will never be used. |
LGTM aside from a super minor nit in a test file. Can you please add a release note for the new diagnostics as well?
I'm happy to land this for you when you're ready, but let me know which email address you'd like me to use this time around.
clang/test/Preprocessor/ifdef-recover.c | ||
---|---|---|
23–29 | Just to keep comment styles the same. |
Updated the code as reviewed, added a release note, and merged ext-cpp2b-pp-directive.cpp & ext-c2x-pp-directive.c into ext-pp-directive.c.
Thank you so much for your review!
My public email address is: 07softy_bride@icloud.com.
Oops, it looks like precommit CI found a relevant issue, can you fix it up?
Failed Tests (1): Clang :: Preprocessor/elifdef.c Testing Time: 790.67s Skipped : 56 Unsupported : 1997 Passed : 87720 Expectedly Failed: 234 Failed : 1
I think we need two diagnostics, one for C2x and one for C++2b (https://wg21.link/p2334 was adopted for C++23). Each of these diagnostics should come in a pair:
and similar for C, except with wording about C standards and in the C warning groups.