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

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r350891 (clang part) and r350892 (clang-tools-extra part). If @rsmith has concerns, we can fix or revert when raised. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D544

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

vmiklos added a comment. @aaron.ballman do you wait for someone else to commit this? Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D54450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

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

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. To me this looks like a reasonably straight-forward refactoring. I'm guessing the initial code had good test coverage, and none of those tests break; and the new code appears to have re

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

vmiklos added a comment. Could you please review this one? It would be especially helpful, due to the depending other review. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D54450 ___ cfe-commits

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

vmiklos added a comment. Ping, could this be reviewed, please? :-) Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D54450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.or

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

aaron.ballman updated this revision to Diff 177508. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Adding tests based on review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D54450 Files: include/clang/Lex/

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

aaron.ballman added inline comments. Comment at: lib/Lex/PPExpressions.cpp:154-156 // Consume the ). -Result.setEnd(PeekTok.getLocation()); PP.LexNonComment(PeekTok); +Result.setEnd(PeekTok.getLocation()); lebedev.ri wrote: > I'm not sure this i

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

lebedev.ri added a comment. This seems to be correct, but the test coverage could be improved. There are no tests with `()` e.g. Comment at: lib/Lex/PPExpressions.cpp:154-156 // Consume the ). -Result.setEnd(PeekTok.getLocation()); PP.LexNonComment(PeekTok); +

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

vmiklos added a comment. Next ping -- could you please review this one? Thanks! :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D54450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://list

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

aaron.ballman added a comment. P-p-power ping! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D54450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

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

vmiklos added a comment. Could you please review this at some stage? As mentioned previously, this is a dependency for D54349 . Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54450/new/ https://reviews.llvm.org/D54450 _

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

aaron.ballman added a comment. Ping. https://reviews.llvm.org/D54450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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-tra

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

aaron.ballman updated this revision to Diff 174041. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Updating based on review feedback. https://reviews.llvm.org/D54450 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPDirectives.cpp lib/Lex/PPExpressions.cpp

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

vmiklos added a comment. 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

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

vmiklos added a comment. As far as I see GCC warns on these 3 things. Comment at: lib/Lex/PPDirectives.cpp:553 } else { const SourceLocation CondBegin = CurPPLexer->getSourceLocation(); // Restore the value of LexingRawMode so that identifiers are

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

aaron.ballman created this revision. aaron.ballman added reviewers: rsmith, dblaikie, echristo. Herald added subscribers: jsji, kbarton, nemanjai. While working on https://reviews.llvm.org/D54349, it was noted that the `SourceRange` returned from the preprocessor callbacks was bogus. It was expe