[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54450#1298319, @vmiklos wrote:

> Oh, and running the `check-clang-tools` target, this currently results in:
>
>   Failing Tests (3):
>       Clang Tools :: modularize/ProblemsInconsistent.modularize
>       Clang Tools :: pp-trace/pp-trace-conditional.cpp
>       Clang Tools :: pp-trace/pp-trace-macro.cpp
>
>
> Perhaps the pp-trace one just has to be updated for the new behavior. :-)


Good catch -- I didn't run the clang-tools-extra tests. The pp-trace tests needed simple updating, but the modularize test has changed behavior and I'm not entirely certain of why. I've attached a file for the clang-tools-extra test changes: F7547470: preproc2.patch <https://reviews.llvm.org/F7547470> I am entirely unfamiliar with the "modularize" tool and there was a surprising change in behavior there, but I think the behavior is still okay (or, at least, no worse than the previous behavior which was already suspicious in that test case).



================
Comment at: lib/Lex/PPDirectives.cpp:553
         } else {
           const SourceLocation CondBegin = CurPPLexer->getSourceLocation();
           // Restore the value of LexingRawMode so that identifiers are
----------------
vmiklos wrote:
> Is `CondBegin` still needed after your changes?
Nope, good catch!


================
Comment at: lib/Lex/PPExpressions.cpp:852
     DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
     return {false, DT.IncludedUndefinedIds};
   }
----------------
vmiklos wrote:
> The new `ExprRange` member is not initialized here, it seems.
> 
`ExprRange` is default initialized in that case, so the default constructor is called as expected.


https://reviews.llvm.org/D54450