This is an archive of the discontinued LLVM Phabricator instance.

[Parser] (C++) Make -Wextra-semi slightly more useful
ClosedPublic

Authored by lebedev.ri on Feb 10 2018, 11:17 AM.

Details

Summary

Let's suppose the -Weverything is passed.

Given code like

void F() {}
;

If the code is compiled with -std=c++03, it would diagnose that extra sema:

<source>:2:1: warning: extra ';' outside of a function is a C++11 extension [-Wc++11-extra-semi]
;
^~

If the code is compiled with -std=c++11, it also would diagnose that extra sema:

<source>:2:1: warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-pedantic]
;
^~

But, let's suppose the C++11 or higher is used, and the used does not care
about -Wc++98-compat-pedantic, so he disables that diagnostic.
And that silences the complaint about extra ; too.
And there is no way to re-enable that particular diagnostic, passing -Wextra-semi does nothing...

Now, there is also a related no newline at end of file diagnostic, which is also emitted by -Wc++98-compat-pedantic

<source>:2:2: warning: C++98 requires newline at end of file [-Wc++98-compat-pedantic]
;
 ^

But unlike the previous case, if -Wno-c++98-compat-pedantic is passed, that diagnostic stays displayed:

<source>:2:2: warning: no newline at end of file [-Wnewline-eof]
;
 ^

This diff refactors the code so -Wc++98-compat-extra-semi can be re-enabled, after the -Wc++98-compat-pedantic was disabled.
This seems ugly, but there does not seem to be any saner way.

Testing: $ ninja check-clang-sema check-clang-semacxx check-clang-lexer check-clang-parser

Diff Detail

Repository
rC Clang

Event Timeline

lebedev.ri created this revision.Feb 10 2018, 11:17 AM

After a brief disscussion with @aaron.ballman in IRC,
the question still stands whether this is really the correct approach,
or is there some nicer (TableGen-based?) approach.

Mainly waiting for @rsmith's opinion.

To be clear -- my concerns were mostly that it seems messy for this sort of thing to require three different diagnostic entries and somewhat convoluted logic to select them -- if we can find a way to generalize this, that'd be better.

This is the wrong way to deal with this. The only thing that should ever be controlled by -W flags is whether the warnings in that group appear, not whether warnings in other groups appear. The principle is that -W flags should behave "as if" they filter the diagnostic output of Clang. (We actually depend on this in the implicit modules implementation, to allow modules to be reused in compilation modes where a different set of warning flags is enabled.) isIgnored exists only to allow the computations leading to the emission of a diagnostic to be skipped if the diagnostic will not actually be emitted.

The right thing to do is to add a new warning group for the warn_cxx98_compat_top_level_semi warning, and make that a subgroup of the -Wextra-semi group. That way -Wno-extra-semi can be used to turn off both the C++98 extension warning and the C++11 compatibility warning. Take a look at the *BinaryLiteral warning groups for an example of how to do this. (That case is a little more complex because there's also a warning group for the corresponding case in C.)

Thank you for the feedback!

This is the wrong way to deal with this. The only thing that should ever be controlled by -W flags is whether the warnings in that group appear, not whether warnings in other groups appear. The principle is that -W flags should behave "as if" they filter the diagnostic output of Clang. (We actually depend on this in the implicit modules implementation, to allow modules to be reused in compilation modes where a different set of warning flags is enabled.) isIgnored exists only to allow the computations leading to the emission of a diagnostic to be skipped if the diagnostic will not actually be emitted.

Aha, i see. To be noted, this is how it is currently already done for warn_cxx98_compat_no_newline_eof/warn_no_newline_eof
https://github.com/llvm-mirror/clang/blob/f828172bcfd7d6d10497c645c3cc5eee321cd669/lib/Lex/Lexer.cpp#L2684-L2695
https://github.com/llvm-mirror/clang/blob/f828172bcfd7d6d10497c645c3cc5eee321cd669/include/clang/Basic/DiagnosticLexKinds.td#L58-L63
So i'm guessing that^ should be fixed too?

The right thing to do is to add a new warning group for the warn_cxx98_compat_top_level_semi warning, and make that a subgroup of the -Wextra-semi group. That way -Wno-extra-semi can be used to turn off both the C++98 extension warning and the C++11 compatibility warning. Take a look at the *BinaryLiteral warning groups for an example of how to do this. (That case is a little more complex because there's also a warning group for the corresponding case in C.)

I'll look into it, but *right now* this sounds like it won't do what i'm trying to do here. Hopefully i'm just missing the point :)

lebedev.ri updated this revision to Diff 136503.Mar 1 2018, 5:46 AM
lebedev.ri edited the summary of this revision. (Show Details)

Reworked stuff via new CXX98CompatExtraSemi diag group.

As expected, passing -Wno-c++98-compat-pedantic disables the extra-semi diag, which exactly the opposite from what i'm trying to do here...
But now that the diag is moved to the new diag group, it can be re-enabled, which is at least something. But this looks kinda ugly.
Is this how it should be?

If i'm reading git blame correctly, the -Wnewline-eof diag, which i used as a base to model the previous version of this diff on,
was added in rL189110 / https://github.com/llvm-mirror/clang/commit/7865b8e4324378e06f59adb4d60bec26a7d3d584 by @jordan_rose.
Since the @rsmith's review note suggests the approach is incorrect, i've added the author of that commit as a subscriber, just in case.

It's been a long time since that commit, but I think the difference is that -pedantic / Extension-type diagnostics are magic and -Wc++98-compat-pedantic is not. I like Richard's way better and if it could be applied to -Wnewline-eof later as well then that sounds great.

Let's move weekly ping away from friday :)
Ping.

aaron.ballman accepted this revision.Mar 14 2018, 7:27 AM

LGTM aside from a testing nit.

test/SemaCXX/extra-semi.cpp
14

Can you use the full diagnostic text for this first appearance in the test?

This revision is now accepted and ready to land.Mar 14 2018, 7:27 AM
lebedev.ri added inline comments.Mar 14 2018, 7:38 AM
test/SemaCXX/extra-semi.cpp
14

Will require preprocessor, because these are two different messages depending of -std=
Will do.

This revision was automatically updated to reflect the committed changes.