[clang-scan-deps] Allow continuation line backslashes followed by whitespace in the dependency source minimizer

[arphaman]

Summary

Clang allows continuations that have whitespace between the backslash and the newline. This patch ensures that the dependency source minimizer can handle the whitespace between the backslash and the newline when looking for a line continuation.

[dexonsmith]

The code looks correct. I have some nitpicks about how the functions are named, but you don't need to go with my suggestions specifically.

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp

247–248

I don't love this name change, since the idea of the function is to take a "first, last" half-open range and reverse "last" back to exclude the spaces. "UntilFirstSpace" suggests that it would stop early and make the half-open range include a space.

Of course it's all a bit weird because it's not returning a range, just the end of it. So the intent of both of these functions is to trim the trailing whitespace, it's just this one points at the beginning of the whitespace to serve as the end of the range, and the other one points at the last thing before the trailing whitespace. Perhaps we can be more explicit about what precisely is being returned, instead of describing intent.

What about findFirstTrailingSpace?

258–264

What about findLastNonSpace?

[dexonsmith]

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp

247–248

You might also reimplement this in terms of the other for clarity:

static const char *findFirstTrailingSpace(const char *First,
                                          const char *Last) {
  const char *LastNonSpace = findLastNonSpace(First, Last);
  if (Last == LastNonSpace)
    return Last;
  assert(isHorizontalWhitespace(LastNonSpace[1]));
  return LastNonSpace + 1;
}

... unless you think that will hurt performance.

clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp

for (;;) {
     if (First[-1] != '\\')
       return;
 
     First += Len;
     // Keep skipping lines...
   }
 }
 
-static const char *reverseOverSpaces(const char *First, const char *Last) {
+static const char *reverseOverSpacesUntilFirstSpace(const char *First,
+                                                    const char *Last) {
   assert(First <= Last);
   const char *PrevLast = Last;
   while (First != Last && isHorizontalWhitespace(Last[-1])) {
     PrevLast = Last;
     --Last;
   }
   return PrevLast;
 }
 
+static const char *reverseOverSpaces(const char *First, const char *Last) {
+  assert(First <= Last);
+  while (First != Last && isHorizontalWhitespace(Last[-1]))
+    --Last;
+  return Last;
+}
+
 static void skipLineComment(const char *&First, const char *const End) {
   assert(First[0] == '/' && First[1] == '/');
   First += 2;
   skipToNewlineRaw(First, End);
 }
 
 static void skipBlockComment(const char *&First, const char *const End) {
   assert(First[0] == '/' && First[1] == '*');
do {
       }
 
       if (Last[1] != '/' && Last[1] != '*') {
         ++Last;
         continue;
       }
 
       // Deal with "//..." and "/*...*/".
-      append(First, reverseOverSpaces(First, Last));
+      append(First, reverseOverSpacesUntilFirstSpace(First, Last));
       First = Last;
 
       if (Last[1] == '/') {
         skipLineComment(First, End);
         return;
       }
 
       put(' ');
       skipBlockComment(First, End);
       skipOverSpaces(First, End);
       Last = First;
     } while (Last != End && !isVerticalWhitespace(*Last));
 
     // Print out the string.
-    if (Last == End || Last == First || Last[-1] != '\\') {
-      append(First, reverseOverSpaces(First, Last));
+    const char *LastBeforeTrailingSpace = reverseOverSpaces(First, Last);
+    if (Last == End || LastBeforeTrailingSpace == First ||
+        LastBeforeTrailingSpace[-1] != '\\') {
+      append(First, LastBeforeTrailingSpace);
       First = Last;
       skipNewline(First, End);
       return;
     }
 
-    // Print up to the backslash, backing up over spaces.
-    append(First, reverseOverSpaces(First, Last - 1));
+    // Print up to the backslash, backing up over spaces. Preserve at least one
+    // space, as the space matters when tokens are separated by a line
+    // continuation.
+    append(First, reverseOverSpacesUntilFirstSpace(
+                      First, LastBeforeTrailingSpace - 1));
 
     First = Last;
     skipNewline(First, End);
     skipOverSpaces(First, End);
   }
 }
 
 static void skipWhitespace(const char *&First, const char *const End) {

clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp

TEST(MinimizeSourceToDependencyDirectivesTest, DefineInvalidMacroArguments) {
   EXPECT_STREQ("#define MACRO(/* invalid */\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, DefineHorizontalWhitespace) {
   SmallVector<char, 128> Out;
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
       "#define MACRO(\t)\tcon \t tent\t", Out));
-  EXPECT_STREQ("#define MACRO() con \t tent\t\n", Out.data());
+  EXPECT_STREQ("#define MACRO() con \t tent\n", Out.data());
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
       "#define MACRO(\f)\fcon \f tent\f", Out));
-  EXPECT_STREQ("#define MACRO() con \f tent\f\n", Out.data());
+  EXPECT_STREQ("#define MACRO() con \f tent\n", Out.data());
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
       "#define MACRO(\v)\vcon \v tent\v", Out));
-  EXPECT_STREQ("#define MACRO() con \v tent\v\n", Out.data());
+  EXPECT_STREQ("#define MACRO() con \v tent\n", Out.data());
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
       "#define MACRO \t\v\f\v\t con\f\t\vtent\v\f \v", Out));
-  EXPECT_STREQ("#define MACRO con\f\t\vtent\v\n", Out.data());
+  EXPECT_STREQ("#define MACRO con\f\t\vtent\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, DefineMultilineArgs) {
   SmallVector<char, 128> Out;
 
   ASSERT_FALSE(
       minimizeSourceToDependencyDirectives("#define MACRO(a        \\\n"
                                            "              )",
TEST(MinimizeSourceToDependencyDirectivesTest, SplitIdentifier) {
   EXPECT_STREQ("#define GUARD\n", Out.data());
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives("#define GUA\\\n"
                                                     "           RD\n",
                                                     Out));
   EXPECT_STREQ("#define GUA RD\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest,
+     WhitespaceAfterLineContinuationSlash) {
+  SmallVector<char, 128> Out;
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives("#define A 1 + \\  \n"
+                                                    "2 + \\\t\n"
+                                                    "3\n",
+                                                    Out));
+  EXPECT_STREQ("#define A 1 + 2 + 3\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, PoundWarningAndError) {
   SmallVector<char, 128> Out;
 
   for (auto Source : {
            "#warning '\n#include <t.h>\n",
            "#warning \"\n#include <t.h>\n",
            "#warning /*\n#include <t.h>\n",
            "#warning \\\n#include <t.h>\n#include <t.h>\n",