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