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

[Corentin Jabot via Phabricator via llvm-commits]

cor3ntin added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:85
+  assert(First != nullptr);
+  if (std::isalpha(*First) || Loc.isMacroID())
+    return;
----------------
cjdb wrote:
> aaron.ballman wrote:
> > `isLetter()`? (or potentially `isIdentifierHead()` if we need to care about dollar-sign prefixed identifiers)
> Is this necessary in the given context? The reason I was confident in using `isalpha` is because we know this is an operator and the only values that this could possibly be are one of `a`, `b`, `c`, `n`, `o`, or `x`.
> 
> (cc @cor3ntin)
Definitively.
Both because trying to make assumption based on the current set of leading symbols in alternative token seems brittle to me.
But mostly because `isalpha` answers "is this byte interpreted in the encoding associated to the current locale a letter". in some scenario (clang running on zOS for example) this will never give the right answer,

I agree with Aaron: use `isIdentifierHead()`

But maybe I'd go further: Have you considered first checking for the opcode first, then the spelling?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294