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