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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:51-54
+  if (!getLangOpts().C99) {
+    return llvm::None;
+  }
+
----------------



================
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 =
----------------
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.)


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:78-79
+                                           const UnaryOperator &Op) {
+  // TODO: make check configurable so users can decide which operators are to be
+  // symbols and which are to be words.
+
----------------
Are you planning to do this as part of this patch (if not, should this switch to FIXME)?


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:85
+  assert(First != nullptr);
+  if (std::isalpha(*First) || Loc.isMacroID())
+    return;
----------------
`isLetter()`? (or potentially `isIdentifierHead()` if we need to care about dollar-sign prefixed identifiers)


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:104
+                                           const BinaryOperator &Op) {
+  // TODO: make check configurable so users can decide which operators are to be
+  // symbols and which are to be words.
----------------
Same here as above.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:111
+  assert(First != nullptr);
+  if (std::isalpha(*First) || Loc.isMacroID())
+    return;
----------------
Same here as above.


================
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;
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:144-148
+  // TODO: make check configurable so users can decide which operators are to be
+  // symbols and which are to be words.
+
+  // TODO: add check to make it possible to "intelligently" determine if symbol
+  // is always preferred (e.g. operator| being used for composition).
----------------
FIXMEs?


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:158-164
+  constexpr Opcode And = Opcode::OO_AmpAmp;
+  constexpr Opcode Bitand = Opcode::OO_Amp;
+  constexpr Opcode Bitor = Opcode::OO_Pipe;
+  constexpr Opcode Compl = Opcode::OO_Tilde;
+  constexpr Opcode Not = Opcode::OO_Exclaim;
+  constexpr Opcode Or = Opcode::OO_PipePipe;
+  constexpr Opcode Xor = Opcode::OO_Caret;
----------------
Nit: given that these are only used once, I'd rather just spell out the `case` labels longhand.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h:35
+                           Preprocessor *ModuleExpanderPP) override {
+    // TODO: add support for checking preprocessor directives
+    Includer.registerPreprocessor(PP);
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/StrictConstCorrectness.h:9
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRICTCONSTCORRECTNESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRICTCONSTCORRECTNESS_H
----------------
Did you mean to include this file in this review?


================
Comment at: clang-tools-extra/clang-tidy/readability/StrictConstCorrectness.kpp:1
+//===-- StringCompareCheck.cpp - clang-tidy--------------------------------===//
+//
----------------
Same for this one?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-alternative-tokens.rst:4
+readability-alternative-tokens
+==================================
+
----------------



================
Comment at: clang/test/Analysis/diagnostics/readability-strict-const-correctness.cpp:1
+// RUN: %check_clang_tidm1 %s readabilitm1-strict-const-correctness %t
+int f1()
----------------
No idea how this managed to trigger a CI failure of `line 1: fg: no job control` :-D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294