[PATCH] D31308: [clang-tidy] new check readability-no-alternative-tokens

Matthias Gehre via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 28 15:07:10 PDT 2017


mgehre added inline comments.


================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:34
+
+  if (const auto *B = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
+    switch (B->getOpcode()) {
----------------
aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > I think this would make more sense lifted into an AST matcher -- there are bound to be a *lot* of binary operators, so letting the matcher memoize things is likely to give better performance.
> > Any reasons not to do this on the lexer level?
> Technical reasons? None.
> User-experience reasons? We wouldn't want this to be on by default (I don't think) and we usually don't implement off-by-default diagnostics in Clang. I think a case could be made for doing it in the Lexer if the performance is really that bad with a clang-tidy check and we couldn't improve it here, though.
Do I correctly understand that "doing this on lexer level" would mean to implement this as a warning directly into clang? If yes, would it be possible to generate fixits and have them possibly applied automatically (as it is the case for clang-tidy)?


================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:68
+  if (PrimarySpelling != Spelling) {
+    diag(OpLoc, "operator uses alternative spelling")
+        << FixItHint::CreateReplacement(TokenRange, PrimarySpelling);
----------------
aaron.ballman wrote:
> This diagnostic doesn't help the user to understand what's wrong with their code (especially in the presence of multiple operators). Perhaps "'%0' is an alternative token spelling; consider using '%1'"
> 
> It would be nice if we could say "consider using %1 for <reason>", but I'm really not certain why we would diagnose this code in the first place (it's purely a matter of stylistic choice, as I understand it).
The main rational for this check is to enforce consistency and thus make it easier to read and comprehend the code.
I agree with your proposed diagnostics.


https://reviews.llvm.org/D31308





More information about the cfe-commits mailing list