[PATCH] D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: clang/include/clang/Basic/CharInfo.h:94
+LLVM_READONLY inline bool isWhitespace(llvm::StringRef S) {
+  for (StringRef::const_iterator I = S.begin(), E = S.end(); I != E; ++I) {
+    if (!isWhitespace(*I))
----------------
I'd suggest to avoid overloading here. A name like `isWhitespaceOnly` would be less ambiguous.

As for implementation, it can be less verbose:
```
for (char C : S)
  if (!isWhitespace(*I))
    return false;
return true;
```
or just
```
return llvm::all_of(S, isWhitespace);
```


================
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:
> kadircet wrote:
> > 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?
> Excellent! Better than the two ideas I thought of. Thanks!
I'd like to draw your attention that `isWhitespace(StringRef)` is somewhat ambiguous. What exactly does it check? That the string is exactly one whitespace character? That it contains a whitespace? That it's only contains whitespace? Maybe `isWhitespaceOnly`? See also the comment in CharInfo.h.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682