[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()) {
----------------
mgehre wrote:
> 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)?
You are correct, that means implementing it as a warning in Clang. I believe you can still generate those fixits from lexer-level diagnostics, but have not verified it.

However, I don't think this diagnostic would be appropriate for Clang because it would have to be off by default.


================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:68
+  if (PrimarySpelling != Spelling) {
+    diag(OpLoc, "operator uses alternative spelling")
+        << FixItHint::CreateReplacement(TokenRange, PrimarySpelling);
----------------
mgehre wrote:
> 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.
I think that enforcing consistency is a good rationale for having the check, but would second the suggestion that this check have an option to enforce the consistency one way or the other.

Then the diagnostic can be:

"'%0' is %select{an alternative token|a primary token}2; consider using '%1' for consistency"


================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:22
+void OperatorsRepresentationCheck::registerMatchers(MatchFinder *Finder) {
+  // We ignore implicit != operators in C++11 range-based for loops.
+  Finder->addMatcher(binaryOperator(unless(hasLHS(ignoringImpCasts(
----------------
I think that this check should only be performed in C++. In C, the alternative token spellings are implemented as macros and would require additional work to support.


https://reviews.llvm.org/D31308