[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:84
+    CheckFactories.registerCheck<RedundantPreprocessorCheck>(
+        "readability-redundant-preprocessor");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
----------------
Please keep this list sorted alphabetically.


================
Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+    StringRef SourceText =
+        Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+                             PP.getSourceManager(), PP.getLangOpts());
+    std::string Condition = getCondition(SourceText);
----------------
Szelethus wrote:
> vmiklos wrote:
> > Szelethus wrote:
> > > 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.
> > 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.
> 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` :/
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.


https://reviews.llvm.org/D54349