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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

The changes are not directly related to clang-tidy, so could you get rid of that clang-tidy some-checker-specific tests and put some unittests into clang/unittests/Format/CleanupTest.cpp ?



================
Comment at: clang/include/clang/Basic/CharInfo.h:196
 
+/// Requires that BufferPtr point to a newline character ("/n" or "/r").
+/// Returns a pointer past the end of any platform newline, i.e. past
----------------
use '\' instead of '/' ?


================
Comment at: clang/include/clang/Basic/CharInfo.h:200
+LLVM_READONLY inline const char *skipNewlineChars(const char *BufferPtr,
+                                                 const char *BufferEnd) {
+  if (BufferPtr == BufferEnd)
----------------
formatting seems to be off, have you run clang-format on your changes?


================
Comment at: clang/include/clang/Basic/CharInfo.h:216
+/// Returns true if and only if S consists entirely of whitespace.
+LLVM_READONLY inline bool isWhitespaceStringRef(llvm::StringRef S) {
+  for (StringRef::const_iterator I = S.begin(), E = S.end(); I != E; ++I) {
----------------
could you make rename this to `isWhitespace` and move it closer to `isWhitespace(unsigned char c)`


================
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) {
----------------
poelmanc wrote:
> 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;
> ```
ah that's unfortunate, I believe it makes sense to have this as an overload though.

Could you instead make the predicate explicit by putting
```[](char c) { return clang::isWhitespace(c); }```
instead of just `clang::isWhitespace` in those call sites?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682