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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:34
+
+  if (const auto *B = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
+    switch (B->getOpcode()) {
----------------
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.


================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:68
+  if (PrimarySpelling != Spelling) {
+    diag(OpLoc, "operator uses alternative spelling")
+        << FixItHint::CreateReplacement(TokenRange, PrimarySpelling);
----------------
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).


================
Comment at: docs/clang-tidy/checks/readability-operators-representation.rst:4
+readability-operators-representation
+=================================
+
----------------
This underline is going to cause Sphinx diagnostics.


================
Comment at: docs/clang-tidy/checks/readability-operators-representation.rst:8
+such as ``not`` (for ``!``), ``bitand`` (for ``&``), ``or`` (for ``||``) or ``not_eq``
+(for ``!=``).
----------------
Why would someone want to do this? You should at least touch on that in the documentation.

Also, this doesn't read very clearly to me. Perhaps it would be better as a table with two columns, the first specifying the alternative token spelling and the second specifying the replacement (with suitable headings),


================
Comment at: test/clang-tidy/readability-operators-representation.cpp:54
+  c = !a;        // OK
+}
----------------
Please add a test where a class overloads the operators. For special fun:
```
struct S {
  friend S& operator and(const S &LHS, const S &RHS);
};

int main() {
  S s1, s2;
  S s3 = s1 and s2;
}
```
I'm not convinced the correct answer here is to tell the user to use a spelling that isn't used by the class definition...


https://reviews.llvm.org/D31308