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

[arphaman]

Summary

The existing string/character literal skipping code in the dependency directives source minimizer has two issues:

• It doesn't stop the scanning when a newline is reached before the terminating character, unlike the lexer which considers the token to be done (even if it's invalid) at the end of the line.
• It doesn't support whitespace between '\' and the newline when looking if the '\' is used as a line continuation character.

[kousikk]

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp

205

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

[dexonsmith]

LGTM, with a few nitpicks about unnecessary nesting.

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp

205

Invert condition with early continue?

206

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.

208

Invert condition with early continue?

215

Is this continue needed?

[kousikk]

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp

206

Ah makes sense, thanks.

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp

if (size_t(Last - First) < Terminator.size())
       continue;
     if (*Last != '"')
       continue;
     First = Last + 1;
     return;
   }
 }
 
-static void skipString(const char *&First, const char *const End) {
-  assert(*First == '\'' || *First == '"' || *First == '<');
-  const char Terminator = *First == '<' ? '>' : *First;
-  for (++First; First != End && *First != Terminator; ++First)
-    if (*First == '\\')
-      if (++First == End)
-        return;
-  if (First != End)
-    ++First; // Finish off the string.
-}
-
 // Returns the length of EOL, either 0 (no end-of-line), 1 (\n) or 2 (\r\n)
 static unsigned isEOL(const char *First, const char *const End) {
   if (First == End)
     return 0;
   if (End - First > 1 && isVerticalWhitespace(First[0]) &&
       isVerticalWhitespace(First[1]) && First[0] != First[1])
     return 2;
   return !!isVerticalWhitespace(First[0]);
 }
 
+static void skipString(const char *&First, const char *const End) {
+  assert(*First == '\'' || *First == '"' || *First == '<');
+  const char Terminator = *First == '<' ? '>' : *First;
+  for (++First; First != End && *First != Terminator; ++First) {
+    // String and character literals don't extend past the end of the line.
+    if (isVerticalWhitespace(*First))
+      return;
+    if (*First != '\\')
+      continue;
+    // Skip past backslash to the next character. This ensures that the
+    // character right after it is skipped as well, which matters if it's
+    // the terminator.
+    if (++First == End)
+      return;
+    if (!isWhitespace(*First))
+      continue;
+    // Whitespace after the backslash might indicate a line continuation.
+    const char *FirstAfterBackslashPastSpace = First;
+    skipOverSpaces(FirstAfterBackslashPastSpace, End);
+    if (unsigned NLSize = isEOL(FirstAfterBackslashPastSpace, End)) {
+      // Advance the character pointer to the next line for the next
+      // iteration.
+      First = FirstAfterBackslashPastSpace + NLSize - 1;
+    }
+  }
+  if (First != End)
+    ++First; // Finish off the string.
+}
+
 // Returns the length of the skipped newline
 static unsigned skipNewline(const char *&First, const char *End) {
   if (First == End)
     return 0;
   assert(isVerticalWhitespace(*First));
   unsigned Len = isEOL(First, End);
   assert(Len && "expected newline");
   First += Len;

clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp

Source = R"(// comment
     #pragma once extra tokens
     // another comment
     #include <test.h>
     )";
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
   EXPECT_STREQ("#pragma once\n#include <test.h>\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest,
+     SkipLineStringCharLiteralsUntilNewline) {
+  SmallVector<char, 128> Out;
+
+  StringRef Source = R"(#if NEVER_ENABLED
+    #define why(fmt, ...) #error don't try me
+    #endif
+
+    void foo();
+)";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ(
+      "#if NEVER_ENABLED\n#define why(fmt,...) #error don't try me\n#endif\n",
+      Out.data());
+
+  Source = R"(#if NEVER_ENABLED
+      #define why(fmt, ...) "quote dropped
+      #endif
+
+      void foo();
+  )";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ(
+      "#if NEVER_ENABLED\n#define why(fmt,...) \"quote dropped\n#endif\n",
+      Out.data());
+}
+
+TEST(MinimizeSourceToDependencyDirectivesTest,
+     SupportWhitespaceBeforeLineContinuationInStringSkipping) {
+  SmallVector<char, 128> Out;
+
+  StringRef Source = "#define X '\\ \t\nx'\nvoid foo() {}";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define X '\\ \t\nx'\n", Out.data());
+
+  Source = "#define X \"\\ \r\nx\"\nvoid foo() {}";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define X \"\\ \r\nx\"\n", Out.data());
+
+  Source = "#define X \"\\ \r\nx\n#include <x>\n";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define X \"\\ \r\nx\n#include <x>\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, CxxModules) {
   SmallVector<char, 128> Out;
   SmallVector<Token, 4> Tokens;
 
   StringRef Source = R"(
     module;
     #include "textual-header.h"