[PATCH] D68436: [clang-scan-deps] Improve string/character literal skipping

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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