This is an archive of the discontinued LLVM Phabricator instance.

Warn if using `elifdef` & `elifndef` in not C2x & C++2b mode
ClosedPublic

Authored by ken-matsui on May 7 2022, 2:37 PM.

Diff Detail

Event Timeline

ken-matsui created this revision.May 7 2022, 2:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 2:37 PM
ken-matsui requested review of this revision.May 7 2022, 2:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 2:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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
3262–3263

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

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

Update the code as reviewed

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.
ExtWarn diagnostics are warnings that are enabled via -pedantic but are also on by default.
Warning diagnostics are warnings. You can use things like DefaultIgnore or DefaultError to control the default behavior of the warning.
Error diagnostics are errors.

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.

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

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?

aaron.ballman added inline comments.May 10 2022, 8:40 AM
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;
ken-matsui added inline comments.May 10 2022, 8:45 AM
clang/include/clang/Basic/DiagnosticLexKinds.td
696–698

Oh, that makes sense. Thank you for your explanation!

ken-matsui retitled this revision from Warn if using `elifdef` & `elifndef` in not C2x mode to Warn if using `elifdef` & `elifndef` in not C2x & C++2b mode.May 10 2022, 8:46 AM

Fix failed tests

aaron.ballman added inline comments.May 11 2022, 4:48 AM
clang/include/clang/Basic/DiagnosticLexKinds.td
701

(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.

Update the code as reviewed

Thank you for your review! I updated the code.

aaron.ballman accepted this revision.May 11 2022, 9:57 AM

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 ↗(On Diff #428636)

Just to keep comment styles the same.

This revision is now accepted and ready to land.May 11 2022, 9:57 AM

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

Fix the failed test

This revision was landed with ongoing or failed builds.May 12 2022, 6:27 AM
This revision was automatically updated to reflect the committed changes.