Page MenuHomePhabricator

Get the correct range of tokens for preprocessor conditions
ClosedPublic

Authored by aaron.ballman on Nov 12 2018, 2:31 PM.

Details

Summary

While working on D54349, it was noted that the SourceRange returned from the preprocessor callbacks was bogus. It was expected to pass the source range for the condition to a #if or #elif directive, but what was actually passed was a much larger range that could even include all of the conditionally skipped tokens.

This patch adjusts the source range passed in to the callbacks to only include the condition range itself by tracking that information a bit better in the preprocessor.

Diff Detail

Event Timeline

aaron.ballman created this revision.Nov 12 2018, 2:31 PM

As far as I see GCC warns on these 3 things.

lib/Lex/PPDirectives.cpp
553

Is CondBegin still needed after your changes?

563

Is CondEnd still needed after your changes?

lib/Lex/PPExpressions.cpp
852

The new ExprRange member is not initialized here, it seems.

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. :-)

aaron.ballman marked 3 inline comments as done.

Updating based on review feedback.

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:

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).

lib/Lex/PPDirectives.cpp
553

Nope, good catch!

lib/Lex/PPExpressions.cpp
852

ExprRange is default initialized in that case, so the default constructor is called as expected.

Could you please review this at some stage? As mentioned previously, this is a dependency for D54349. Thanks!

P-p-power ping!

Next ping -- could you please review this one? Thanks! :-)

This seems to be correct, but the test coverage could be improved.
There are no tests with () e.g.

lib/Lex/PPExpressions.cpp
154–157

I'm not sure this is covered with the test?

aaron.ballman marked 3 inline comments as done.

Adding tests based on review feedback.

lib/Lex/PPExpressions.cpp
154–157

The tests for that were attached as a file rather than a separate patch. Basically, this change is needed to keep the behavior the same for tools like pp-trace and modularize. However, I have added new unit tests that demonstrate the behavior.

Ping, could this be reviewed, please? :-) Thanks!

Could you please review this one? It would be especially helpful, due to the depending other review. Thanks!

lebedev.ri accepted this revision.Thu, Jan 3, 1:24 PM

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 reasonable test coverage.
(Mind you, i'm not a code owner of that code.)

This revision is now accepted and ready to land.Thu, Jan 3, 1:24 PM

@aaron.ballman do you wait for someone else to commit this? Thanks.

aaron.ballman closed this revision.Thu, Jan 10, 1:27 PM

I've commit in r350891 (clang part) and r350892 (clang-tools-extra part). If @rsmith has concerns, we can fix or revert when raised.