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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
