Page MenuHomePhabricator

[clang-scan-deps] Dependency directives source minimizer: single quotes are not digit separators after a valid character literal prefix
ClosedPublic

Authored by arphaman on Jul 10 2019, 11:42 AM.

Details

Summary

The single quote character can act as a c++ digit separator. However, the minimizer shouldn't treat it as such when it's actually following a valid character literal prefix, like L, U, u, or u8.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jul 10 2019, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 11:42 AM
dexonsmith accepted this revision.Jul 10 2019, 11:48 AM

LGTM, but I have a suggestion inline for another approach.

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
265–272 ↗(On Diff #209035)

I wonder if this would be easier to identify walking forward from Start rather than working backwards from Cur.

This revision is now accepted and ready to land.Jul 10 2019, 11:48 AM
Bigcheese added inline comments.Jul 10 2019, 12:35 PM
clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
270 ↗(On Diff #209035)

Are we sure at this point that it's always safe to jump back 2?

arphaman marked an inline comment as done.Jul 10 2019, 12:52 PM
arphaman added inline comments.
clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
265–272 ↗(On Diff #209035)

Yes, that's a possible option. We can scan scanning from start, until we reach non-whitespace right before Cur, and then identify the token.

In such ambiguous cases it might be a good idea to raw lex the line using the Lexer from Start to End. Then we'll match the behavior of Lexer when there's an actual error as well. I'll see if I can setup a fallback like this in a follow-up patch.

arphaman marked an inline comment as done.Jul 10 2019, 12:53 PM
arphaman added inline comments.
clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
270 ↗(On Diff #209035)

It should be, because otherwise Start would've been equals to Cur - 1 which we check for right before the dereference.

Bigcheese accepted this revision.Jul 10 2019, 12:59 PM
Bigcheese added inline comments.
clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
270 ↗(On Diff #209035)

Oh, obviously. I missed that check.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 3:03 PM