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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
