[PATCH] D68436: [clang-scan-deps] Improve string/character literal skipping
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 3 23:58:10 PDT 2019
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
LGTM, with a few nitpicks about unnecessary nesting.
================
Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:205
+ return;
+ if (*First == '\\') {
+ if (++First == End)
----------------
kousikk wrote:
> Should you also check if the character right after a backslash is equal to Terminator and if it is, continue on without terminating? The case I'm thinking of is:
>
> ```
> #define FOO "FOO \"doublequote\""
> ```
>
> The testcase would be something like:
>
> ```
> StringRef Source = "#define FOO \"FOO \\\"doublequote\\\"\"
> ... do rest
> ```
>
Invert condition with early `continue`?
================
Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:206
+ if (*First == '\\') {
+ if (++First == End)
+ return;
----------------
> Should you also check if the character right after a backslash is equal to Terminator and if it is, continue on without terminating?
>
@kousikk, that was handled before this patch. This `++First` combined with the one in the loop increment handles skipping over an escaped terminator.
================
Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:208
+ return;
+ if (isWhitespace(*First)) {
+ const char *FirstAfterBackslashPastSpace = First;
----------------
Invert condition with early `continue`?
================
Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:215
+ First = FirstAfterBackslashPastSpace + NLSize - 1;
+ continue;
+ }
----------------
Is this `continue` needed?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68436/new/
https://reviews.llvm.org/D68436
More information about the cfe-commits
mailing list