vmiklos marked 2 inline comments as done.
vmiklos added inline comments.
================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:84
+ CheckFactories.registerCheck<RedundantPreprocessorCheck>(
+ "readability-redundant-preprocessor");
CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
----------------
aaron.ballman wrote:
> Please keep this list sorted alphabetically.
Done.
================
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);
----------------
aaron.ballman wrote:
> 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.
Thanks! I've now rebased this on top of D54450, to be able to drop the ugly
`getCondition()` function.
https://reviews.llvm.org/D54349
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
