Page MenuHomePhabricator

[clang-tidy] new check 'readability-redundant-preprocessor'
ClosedPublic

Authored by vmiklos on Nov 9 2018, 1:19 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

vmiklos created this revision.Nov 9 2018, 1:19 PM
JonasToth added a project: Restricted Project.

I've used this check originally on LibreOffice (core, online) code and it found a handful of not necessary ifdef / ifndef lines. It seemed it's simple and generic enough that it would make sense to upstream this.

No one will know for sure what "pp" in "readability-redundant-pp" means.
I'd highly recommend to fully spell it out.

Also, i'd like to see some analysis of the false-positives.

Hi vmiklos,

thank you for working on this patch!
I added a few comments.

clang-tidy/readability/ReadabilityTidyModule.cpp
83 ↗(On Diff #173432)

I think that name is not very descriptive for the user of clang-tidy. pp should be preprocessor or some other constellation that makes it very clear its about the preprocessor.

clang-tidy/readability/RedundantPpCheck.cpp
28 ↗(On Diff #173432)

you are in namespace clang, you can ellide clang::

37 ↗(On Diff #173432)

Maybe SmallVector<Entry, 4>? Would be better for performance.

48 ↗(On Diff #173432)

I think it would be better to have these methods defined inline, as the splitting does not achieve its original goal (declaration in header, definition in impl).

52 ↗(On Diff #173432)

The two functions are copied, please remove this duplicatoin and refactor it to a general parametrized function.

docs/ReleaseNotes.rst
70 ↗(On Diff #173432)

Please order the checks alphabetically in the release notes, and one empty line at the end is enough.

docs/clang-tidy/checks/readability-redundant-pp.rst
8 ↗(On Diff #173432)

This needs more explanation, please add .. code-block:: c++ sections for the cases that demonstrate the issue.

test/clang-tidy/readability-redundant-pp-ifdef.cpp
6 ↗(On Diff #173432)

Please add a test where the redundancy comes from including other headers. If the headers are nested this case might still occur, but its not safe to diagnose/remove these checks as other include-places might not have the same constellation.

ifdef and ifndef are used for the include-guards and therefore particularly necessary to test.

No one will know for sure what "pp" in "readability-redundant-pp" means.
I'd highly recommend to fully spell it out.

Will do.

Also, i'd like to see some analysis of the false-positives.

Things I considered:

  • header guards would easily generate "nested ifndef" false positives, so I limited the check to the main file only
  • if there are nested #ifdef FOO .. #endif blocks, but FOO is not defined, we fail to detect the redundancy.
  • I read that in general checks should be careful around templates and macros, but given this deals with the preprocessor, I don't expect issues there.

This implicitly means that I'm not aware of actual false positives of the check in its current form.

Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
73 ↗(On Diff #173432)

preprocessor directives? Same in documentation.

docs/clang-tidy/checks/readability-redundant-pp.rst
8 ↗(On Diff #173432)

Please use ``. Same below.

vmiklos updated this revision to Diff 173465.Nov 9 2018, 4:11 PM
vmiklos retitled this revision from [clang-tidy] new check 'readability-redundant-pp' to [clang-tidy] new check 'readability-redundant-preprocessor'.

I think that name is not very descriptive for the user of clang-tidy. pp
should be preprocessor or some other constellation that makes it very clear
its about the preprocessor.

Done, renamed to readability-redundant-preprocessor.

you are in namespace clang, you can ellide clang::

Done.

Maybe SmallVector<Entry, 4>? Would be better for performance.

Done.

I think it would be better to have these methods defined inline, as the
splitting does not achieve its original goal (declaration in header,
definition in impl).

Done.

The two functions are copied, please remove this duplicatoin and refactor it
to a general parametrized function.

Done.

Please order the checks alphabetically in the release notes, and one empty
line at the end is enough.

Done.

This needs more explanation, please add .. code-block:: c++ sections for
the cases that demonstrate the issue.

Done.

Please add a test where the redundancy comes from including other headers. If
the headers are nested this case might still occur, but its not safe to
diagnose/remove these checks as other include-places might not have the same
constellation.

ifdef and ifndef are used for the include-guards and therefore
particularly necessary to test.

Done. I've added a test to make sure we don't warn in headers, the code for
this was already there, just had no coverage. (Exactly for the reason you
mention, the possibility of include-guards generating false positives.)

vmiklos updated this revision to Diff 173468.Nov 9 2018, 4:16 PM
vmiklos edited the summary of this revision. (Show Details)

preprocessor directives? Same in documentation.

Done.

What do you think about code like:

#if FOO == 4
#if FOO == 4
#endif
#endif

#if defined(FOO)
#if defined(FOO)
#endif
#endif

#if !defined(FOO)
#if !defined(FOO)
#endif
#endif

#if defined(FOO)
#if !defined(FOO)
#endif
#endif

#if !defined(FOO)
#if defined(FOO)
#endif
#endif
clang-tidy/readability/CMakeLists.txt
23 ↗(On Diff #173465)

Please keep this list sorted alphabetically.

clang-tidy/readability/RedundantPreprocessorCheck.cpp
52 ↗(On Diff #173465)

This name is not particularly descriptive. This seems to be more like CheckMacroRedundancy or something like that?

clang-tidy/readability/RedundantPreprocessorCheck.h
1–2 ↗(On Diff #173465)

This comment should be re-flowed to fit the column width.

20 ↗(On Diff #173465)

What constitutes "redundancy"? A bit more exposition here would be useful.

Done.

Please also mark the inline comments as done. Otherwise its hard to follow if there are outstanding issues somewhere, especially if code moves around.

Szelethus added inline comments.
clang-tidy/readability/RedundantPreprocessorCheck.cpp
19–22 ↗(On Diff #173468)

This is a way too general name in my opinion. Either include comments that describe it, or rename it (preferably both).

vmiklos updated this revision to Diff 173524.EditedNov 10 2018, 12:22 PM

What do you think about code like:

#if FOO == 4
#if FOO == 4
#endif
#endif

#if defined(FOO)
#if defined(FOO)
#endif
#endif

#if !defined(FOO)
#if !defined(FOO)
#endif
#endif

I looked at supporting these, but it's not clear to me how to get e.g. the 'FOO == 4' string (the condition) from the If() callback. So far I only see how to get the start location of the condition and the whole range till the end of endif. Do you have a code pointer how to do this, please? (Or OK if I leave this for a follow-up patch?)

#if defined(FOO)
#if !defined(FOO)
#endif
#endif

#if !defined(FOO)
#if defined(FOO)
#endif
#endif

I expect handling these correctly is more complex -- I would prefer focusing on matching conditons in this patch, and get back to inverted conditions in a follow-up patch. Is that OK? If we handle inverted conditions, then handling a && b and !a || !b would make sense as well for example.

Please keep this list sorted alphabetically.

Done.

This name is not particularly descriptive. This seems to be more like
CheckMacroRedundancy or something like that?

Makes sense, done.

This comment should be re-flowed to fit the column width.

Done.

What constitutes "redundancy"? A bit more exposition here would be useful.

Hopefully "nested directives with the same condition" makes it easier to understand the intention and current scope.

vmiklos marked 7 inline comments as done.Nov 10 2018, 12:26 PM
vmiklos marked an inline comment as done.Nov 10 2018, 12:31 PM
vmiklos added inline comments.
clang-tidy/readability/RedundantPreprocessorCheck.cpp
19–22 ↗(On Diff #173468)

Renamed to PreprocessorCondition, hope it helps. :-)

vmiklos updated this revision to Diff 173526.Nov 10 2018, 12:31 PM
vmiklos marked an inline comment as done.
Szelethus added inline comments.Nov 10 2018, 12:37 PM
clang-tidy/readability/RedundantPreprocessorCheck.cpp
19–22 ↗(On Diff #173468)

I actually meant it for Entry, if you hover your mouse over an inline comment, you can see which lines it applies to. Sorry for the confusing communication :D

What do you think about code like:

#if FOO == 4
#if FOO == 4
#endif
#endif

#if defined(FOO)
#if defined(FOO)
#endif
#endif

#if !defined(FOO)
#if !defined(FOO)
#endif
#endif

I looked at supporting these, but it's not clear to me how to get e.g. the 'FOO == 4' string (the condition) from the If() callback. So far I only see how to get the start location of the condition and the whole range till the end of endif. Do you have a code pointer how to do this, please? (Or OK if I leave this for a follow-up patch?)

I think it requires manual work. There's a FIXME in PPCallbacks::If() to pass a list/tree of tokens that would make implementing this reasonable. I'd say it's fine as a follow-up patch.

#if defined(FOO)
#if !defined(FOO)
#endif
#endif

#if !defined(FOO)
#if defined(FOO)
#endif
#endif

I expect handling these correctly is more complex -- I would prefer focusing on matching conditons in this patch, and get back to inverted conditions in a follow-up patch. Is that OK? If we handle inverted conditions, then handling a && b and !a || !b would make sense as well for example.

I agree that it's more complex, but that's why I was asking for it -- I don't think the design you have currently will extend for these sort of cases, and I was hoping to cover more utility with the check to hopefully shake out those forward-looking design considerations. As it stands, I feel like this check is a bit too simplistic to have much value, but if you covered some of these other cases, it would alleviate that worry for me.

vmiklos updated this revision to Diff 173554.Nov 11 2018, 5:20 AM
vmiklos marked an inline comment as done.Nov 11 2018, 5:23 AM

As it stands, I feel like this check is a bit too simplistic to have much value, but if you covered some of these other cases, it would alleviate that worry for me.

I've now added support for detecting inverted conditions when it comes to #ifdef and #ifndef (see extended documentation and testcases). I'll also check what can I do for #if. I think it would not be too hard to detect just repeated conditions. if I have the full range, then I can look for end of the first line that does not end with \, and that would be a poor man's way to get the #if condition. Let's see if that works in practice.

clang-tidy/readability/RedundantPreprocessorCheck.cpp
19–22 ↗(On Diff #173468)

Done now. Nah, it's more about I'm not so fluent with phabricator. :-)

vmiklos updated this revision to Diff 173556.Nov 11 2018, 7:27 AM
vmiklos marked an inline comment as done.

Let's see if that works in practice.

I've implemented this now, please take a look. (Extended test + docs as well, as usual.) Thanks!

JonasToth added inline comments.Nov 11 2018, 10:01 AM
docs/clang-tidy/checks/readability-redundant-preprocessor.rst
34 ↗(On Diff #173556)

The indendation for these examples (following as well) is not enough. Please use 2 spaces.

vmiklos updated this revision to Diff 173572.Nov 11 2018, 12:37 PM
vmiklos marked an inline comment as done.
vmiklos added inline comments.
docs/clang-tidy/checks/readability-redundant-preprocessor.rst
34 ↗(On Diff #173556)

Fixed.

Szelethus added inline comments.Nov 11 2018, 12:51 PM
clang-tidy/readability/RedundantPreprocessorCheck.cpp
56–59 ↗(On Diff #173572)

I'm a little confused. To me, it seems like you acquired the condition already -- doesn't ConditionRange actually cover the, well, condition range? This is how I imagined it:

#ifdef CUTE_PANDA_CUBS
       ^~~~~~~~~~~~~~~
       ConditionRange

Why is there a need for getCondition? Is there any? If there is (maybe the acquired text contains other things), can you document it? I haven't played with PPCallbacks much, so I'm fine with being in the wrong.

vmiklos marked 2 inline comments as done.Nov 11 2018, 1:04 PM
vmiklos added inline comments.
clang-tidy/readability/RedundantPreprocessorCheck.cpp
56–59 ↗(On Diff #173572)

ConditionRange covers more than what you expect:

#if FOO == 4
   ^~~~~~~~~
void f();
~~~~~~~~~
#endif
~~~~~~

to find out if the condition of the #if is the same as a previous one, I want to extract just FOO == 4 from that, then deal with that part similar to #ifdef and #ifndef, which are easier as you have a single Token for the condition. But you're right, I should add a comment explaining this.

vmiklos updated this revision to Diff 173575.Nov 11 2018, 1:05 PM
vmiklos marked an inline comment as done.
Szelethus added inline comments.Nov 11 2018, 1:13 PM
clang-tidy/readability/RedundantPreprocessorCheck.cpp
56–59 ↗(On Diff #173572)

Oh my god. There is no tool or a convenient method for this??? I had the displeasure of working with the preprocessor in the last couple months, and I get shocked by things like this almost every day.

Yea, unfortunately you will have to write excessive amount of comments to counterweights the shortcomings of Preprocessor :/

aaron.ballman added inline comments.Nov 11 2018, 6:34 PM
clang-tidy/readability/ReadabilityTidyModule.cpp
84 ↗(On Diff #173575)

Please keep this list sorted alphabetically.

clang-tidy/readability/RedundantPreprocessorCheck.cpp
56–59 ↗(On Diff #173572)

This is working around a bug, and I think it would be better to fix that bug instead of jump through these hoops here.

Preprocessor::EvaluateDirectiveExpression() needs to squirrel away the condition range in the DirectiveEvalResult it returns. I'll take a stab at it and report back.

vmiklos marked 2 inline comments as done.Nov 14 2018, 5:36 AM
vmiklos added inline comments.
clang-tidy/readability/ReadabilityTidyModule.cpp
84 ↗(On Diff #173575)

Done.

clang-tidy/readability/RedundantPreprocessorCheck.cpp
56–59 ↗(On Diff #173572)

Thanks! I've now rebased this on top of D54450, to be able to drop the ugly getCondition() function.

vmiklos updated this revision to Diff 174021.Nov 14 2018, 5:36 AM
vmiklos marked 2 inline comments as done.
aaron.ballman added inline comments.Nov 14 2018, 11:02 AM
clang-tidy/readability/RedundantPreprocessorCheck.cpp
37 ↗(On Diff #174021)

I think these diagnostics should be hoisted as private constant members of the class. Something like:
nested redundant %select{#if|#ifdef|#ifndef}0; consider removing it" and previous %select{#if|#ifdef|#ifndef}0 here

test/clang-tidy/readability-redundant-preprocessor.cpp
45 ↗(On Diff #174021)

Can you add a test like #if FOO == 3 + 1 as well?

vmiklos updated this revision to Diff 174439.Nov 16 2018, 1:18 PM
vmiklos marked 2 inline comments as done.
vmiklos added inline comments.
clang-tidy/readability/RedundantPreprocessorCheck.cpp
37 ↗(On Diff #174021)

Done, I've also added an enum to specify if/ifdef/ifndef to avoid hard-to-read 0/1/2 for these.

aaron.ballman added inline comments.Nov 16 2018, 1:33 PM
test/clang-tidy/readability-redundant-preprocessor.cpp
53 ↗(On Diff #174439)

I didn't describe my test scenario very well -- can you change this line to FOO == 4 but leave the preceding conditional check as FOO == 3 + 1? The goal is to test that this isn't a token-by-token check, but uses the symbolic values instead.

vmiklos marked an inline comment as done.Nov 16 2018, 2:04 PM
vmiklos added inline comments.
test/clang-tidy/readability-redundant-preprocessor.cpp
53 ↗(On Diff #174439)

That doesn't work at the moment, since PreprocessorEntry::Condition is just a string, so 3 + 1 won't equal to 4. I think we discussed this above already, and you said:

I think it requires manual work. There's a FIXME in PPCallbacks::If() to pass a list/tree of tokens that would make implementing this reasonable. I'd say it's fine as a follow-up patch.

But later you wrote:

I agree that it's more complex, but that's why I was asking for it -- I don't think the design you have currently will extend for these sort of cases, and I was hoping to cover more utility with the check to hopefully shake out those forward-looking design considerations. As it stands, I feel like this check is a bit too simplistic to have much value, but if you covered some of these other cases, it would alleviate that worry for me.

My hope is that the check with its current "feature set" already has some value, but let me know if I'm too optimistic. :-)

That being said, if I want to support simple cases like "realize that 3 + 1 and 4 is the same" -- do you imagine that would be manually handled in this check or there is already some reusable building block in the codebase to evaluate if two expressions have the same integer value? I guess doing this manually is a lot of work, e.g. the answer for FOO + 4 would be "it depends", so that should not equal to 8, even if FOO happens to be 4, etc.

aaron.ballman accepted this revision.Nov 16 2018, 2:07 PM

I think this check is in good shape for the initial commit. Additional functionality can be added incrementally.

test/clang-tidy/readability-redundant-preprocessor.cpp
53 ↗(On Diff #174439)

That's a good point -- sorry about the mistake on my suggestion that this test should pass, I had a temporary lapse. :-)

This revision is now accepted and ready to land.Nov 16 2018, 2:07 PM

I think this check is in good shape for the initial commit. Additional functionality can be added incrementally.

OK, thanks. I'll lcommit this once D54450 is committed.

This revision was automatically updated to reflect the committed changes.