[PATCH] D107294: [clang-tidy] adds warning to suggest users replace symbols with words

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

The for Pipe/BitwiseOr issue. One heuristic could be:

- If it has any kind of template dependence don't diagnost
- If its a `BinaryOperator`, its safe to diagnose.
- If its a `CxxOperatorCallExpr`, the simplest case I can think of is to check the spelling of the operator (`operator|` vs `operator bitor`)



================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:69
+  // Only insert spaces if there aren't already spaces between operators
+  StringRef SpaceBefore = std::isspace(lookahead(SM, Loc, -1)) ? "" : " ";
+  StringRef SpaceAfter =
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Hrmm, I feel like `std::isspace()` is not going to properly handle all the Unicode whitespace, but I don't think we handle them all anyway as I notice the lexer is using `isHorizontalWhitespace()`, `isVerticalWhitespace()`, and `isWhitespace()` from `CharInfo.h`. Maybe we should use the same utility here just to match the lexer? (I don't feel strongly, just tickled my spidey senses.)
> I just saw this comment randomly in my mail.
> I agree we should not use `isspace` - it's also probably less efficient because of locale. And we don't want lexing to depend on the host locale.
> And makes it easier to extend the set of supported whitespace if we ever do thatt
Is it not just wiser to always add the spaces and just let clang-format do its thing.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:117-120
+  case Opcode::BO_LAnd:
+    diag(Loc, "use 'and' for logical conjunctions")
+        << createReplacement(SM, Loc, "and", 2) << includeIso646(SM, Loc);
+    break;
----------------
aaron.ballman wrote:
> I think these can be replaced with a helper function along these lines:
> ```
> void emitDiag(const SourceManager &SM, SourceLocation Loc, StringRef Op, StringRef OpDesc, int Lookahead) {
>   diag(Loc, ("use '" + Op + "' for " + OpDesc).str()) <<
>     createReplacement(SM, Loc, Op, Lookahead) << includeIso646(SM, Loc);
> }
> ```
> WDYT?
```lang=c++
diag("use '%0' for %1") << Op << OpDesc << Fixes;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294