[PATCH] D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc marked 4 inline comments as done.
poelmanc added inline comments.


================
Comment at: clang/lib/AST/CommentLexer.cpp:20
+
+// Consider moving this useful function to a more general utility location.
+const char *skipNewline(const char *BufferPtr, const char *BufferEnd) {
----------------
gribozavr2 wrote:
> Please do so in this change. clang/include/clang/Basic/CharInfo.h?
Done. I renamed to skipNewlineChars to remind callers it might skip multiple (1 or 2) characters, and to avoid a name clash with very similar function skipNewline helper function at DependencyDirectivesSourceMinimizer.cpp:228. That version modifies its first argument and returns the number of characters skipped (0, 1, or 2.)

We could alternatively remove DependencyDirectivesSourceMinimizer.cpp's local skipNewline() helper function and call this version, but one location (line 370) would require a few lines of logic that I don't know how to test.


================
Comment at: clang/lib/AST/CommentParser.cpp:19
 
-static inline bool isWhitespace(llvm::StringRef S) {
+// Consider moving this useful function to a more general utility location.
+bool isWhitespace(llvm::StringRef S) {
----------------
gribozavr2 wrote:
> clang/include/clang/Basic/CharInfo.h ?
Done. I renamed it to `isWhitespaceStringRef` to avoid making it an overload of the existing `isWhitespace(unsigned char)`, which causes ambiguous overload errors at QueryParser.cpp:42 & CommentSema.cpp:237.

We could alternatively keep this as an `isWhitespace` overload and instead change those two lines to use a `static_cast<bool (*)(unsigned char)>(&clang::isWhitespace)` or precede them with a line like:
```
bool (*isWhitespaceOverload)(unsigned char) = &clang::isWhitespace;
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682