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

[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.

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.



================
Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:247-248
 
-static const char *reverseOverSpaces(const char *First, const char *Last) {
+static const char *reverseOverSpacesUntilFirstSpace(const char *First,
+                                                    const char *Last) {
   assert(First <= Last);
----------------
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`?


================
Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:258-264
+static const char *reverseOverSpaces(const char *First, const char *Last) {
+  assert(First <= Last);
+  while (First != Last && isHorizontalWhitespace(Last[-1]))
+    --Last;
+  return Last;
+}
+
----------------
What about `findLastNonSpace`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68052/new/

https://reviews.llvm.org/D68052