[clang-tidy] new check readability-no-alternative-tokens

[PiotrZSL]

Summary

Flags (and replaces) the alternative tokens for binary and unary
operators, such as `not (for !), bitand (for &), or (for ||`)
or `not_eq (for !=`).

[Eugene.Zelenko]

I think will be good idea to make this check working in both ways (standard or alternative operator representations), depending on option.

Probably readability-operators-representation will be better name for check.

See also PR25195.

[mgehre]

Thanks for your feedback, Eugene!

I'm not sure if it would be helpful to have this check in both ways. I did a code search for "not_eq", "bitand" and "and_eq"
on github, and their usage seems to be a clear minority.

So I would propose to keep the features as-is for now,
change the name to readability-operators-representation, and then later (someone else?) might also add an option
for making this work the other way around. Would that be ok for you?

Were you refering to https://reviews.llvm.org/D25195 ("[lit] Fix FormatError on individual test timeout")?

[alexfh]

In D31308#709709, @mgehre wrote:

Thanks for your feedback, Eugene!

I'm not sure if it would be helpful to have this check in both ways. I did a code search for "not_eq", "bitand" and "and_eq"
on github, and their usage seems to be a clear minority.

So I would propose to keep the features as-is for now,
change the name to readability-operators-representation, and then later (someone else?) might also add an option
for making this work the other way around. Would that be ok for you?

Were you refering to https://reviews.llvm.org/D25195 ("[lit] Fix FormatError on individual test timeout")?

Nope, "PR" notation is used to reference bugs ("Problem Reports", PRobably): http://llvm.org/PR25195

[alexfh]

So I would propose to keep the features as-is for now,
change the name to readability-operators-representation, and then later (someone else?) might also add an option
for making this work the other way around. Would that be ok for you?

Fine by me.

[alexfh]

FWIW, I'm pretty sure this can and should be done on the lexer level - it will be faster and more universal.

[• gnzlbg]

Thanks for working on this!

I'm not sure if it would be helpful to have this check in both ways. I did a code search for "not_eq", "bitand" and "and_eq" on github, and their usage seems to be a clear minority.

I actually was requesting the opposite version of this (but suggesting to implement both), because for me "if (!something)" is much harder to read than "if (not something)" (I am a bit blind).

So I would propose to keep the features as-is for now, change the name to readability-operators-representation, and then later (someone else?) might also add an option for making this work the other way around. Would that be ok for you?

Sounds good to me. This solves half of the problem, and I agree with you that more people will benefit from this check than from the opposite check. Thanks again for working on this!

[mgehre]

Rename check to readability-operators-representation

[aaron.ballman]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

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.

69

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

docs/clang-tidy/checks/readability-operators-representation.rst

5

This underline is going to cause Sphinx diagnostics.

9

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),

test/clang-tidy/readability-operators-representation.cpp

55

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

[alexfh]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

Any reasons not to do this on the lexer level?

[aaron.ballman]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

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.

[mgehre]

Improved diagnostic; updated documentation; added test for overloaded operator

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

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)?

69

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.

[aaron.ballman]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

22

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.

35

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.

69

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"

[alexfh]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

Actually, I was thinking about changing this clang-tidy check to analyze token stream somehow (probably by handling PPCallbacks to detect ranges that need to be re-lexed) instead of matching the AST. I didn't intend to propose a new Clang warning (but it looks like the wording was misleading).

[mgehre]

only check C++ code; only match operators that can have alternative representations

[aaron.ballman]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

There is some value in that -- it means we could support C, for instance. I'm not certain how easy or hard it would be, but suspect it's reasonable. However, in C, there's still the problem of the include file that introduces those macros. Do we have facilities to remove an include in clang-tidy?

69

The diagnostic is improved, but there's still no way to go the opposite direction (from primary to alternative).

[Eugene.Zelenko]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

Yes, it may make sense in C, but parameter for map from macro name to operator is needed.

Product I'm working for, with long history, had alternative operator presentation implemented in C.

[alexfh]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

Changing macro-based "alternative tokens" to the corresponding operators can also be formulated in the terms of expanding a set of macros, which should work with any LangOpts and arbitrary alternative operator names.

However, in C, there's still the problem of the include file that introduces those macros.

Sounds like a generic "remove unused includes" problem, since we need to analyze whether the header is needed for anything else. In particular, even if the use of the header is limited to the alternative representations of operators, we can't remove the include until we replace all these macros. Clang-tidy doesn't provide any support for transactional fixes and dependencies between fixes.

[aaron.ballman]

clang-tidy/readability/OperatorsRepresentationCheck.cpp

35

Sounds like a generic "remove unused includes" problem, since we need to analyze whether the header is needed for anything else. In particular, even if the use of the header is limited to the alternative representations of operators, we can't remove the include until we replace all these macros. Clang-tidy doesn't provide any support for transactional fixes and dependencies between fixes.

That's a good point. I also don't think an unused include is sufficiently awful to block this.

[Prazek]

Yep, I am also a fan of "and","or" and "not". The other tokens doesn't make it more readable imho

[alexfh]

As noted above, my concern with the current implementation is that the use of AST in this check seems to be superfluous. It should be enough to handle PPCallbacks and re-lex each file opened during compilation to find all alternative tokens.

[PiotrZSL]

Obsolete by D144522

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
+  OperatorsRepresentationCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp

clang-tidy/readability/OperatorsRepresentationCheck.h

+//===--- OperatorsRepresentationCheck.h - clang-tidy-----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_OPERATORS_REPRESENTATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_OPERATORS_REPRESENTATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Flags alternative tokens for operators, such as 'compl', 'not' and 'or'.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-operators-representation.html
+class OperatorsRepresentationCheck : public ClangTidyCheck {
+public:
+  OperatorsRepresentationCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_OPERATORS_REPRESENTATION_H

clang-tidy/readability/OperatorsRepresentationCheck.cpp

+//===--- OperatorsRepresentationCheck.cpp - clang-tidy---------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "OperatorsRepresentationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void OperatorsRepresentationCheck::registerMatchers(MatchFinder *Finder) {
+  // We ignore implicit != operators in C++11 range-based for loops.
+  Finder->addMatcher(binaryOperator(unless(hasLHS(ignoringImpCasts(
+                                        declRefExpr(to(isImplicit()))))))
+                         .bind("binary"),
+                     this);
+  Finder->addMatcher(unaryOperator().bind("unary"), this);
+}
+
+void OperatorsRepresentationCheck::check(const MatchFinder::MatchResult &Result) {
+  StringRef PrimarySpelling;
+  SourceLocation OpLoc;
+
+  if (const auto *B = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
+    switch (B->getOpcode()) {
+    case BO_NE:
+    case BO_And:
+    case BO_Xor:
+    case BO_Or:
+    case BO_LAnd:
+    case BO_LOr:
+    case BO_AndAssign:
+    case BO_OrAssign:
+    case BO_XorAssign:
+      break;
+    default:
+      return;
+    }
+    PrimarySpelling = B->getOpcodeStr();
+    OpLoc = B->getOperatorLoc();
+  }
+
+  if (const auto *U = Result.Nodes.getNodeAs<UnaryOperator>("unary")) {
+    auto OpCode = U->getOpcode();
+    if (OpCode != UO_Not && OpCode != UO_LNot)
+      return;
+    PrimarySpelling = UnaryOperator::getOpcodeStr(OpCode);
+    OpLoc = U->getOperatorLoc();
+  }
+
+  auto &SM = *Result.SourceManager;
+  OpLoc = SM.getSpellingLoc(OpLoc);
+
+  auto TokenRange = CharSourceRange::getTokenRange(OpLoc);
+  StringRef Spelling = Lexer::getSourceText(TokenRange, SM, getLangOpts());
+
+  if (PrimarySpelling != Spelling) {
+    diag(OpLoc, "'%0' is an alternative token spelling; consider using '%1'")
+        << Spelling << PrimarySpelling
+        << FixItHint::CreateReplacement(TokenRange, PrimarySpelling);
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
+#include "OperatorsRepresentationCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "readability-implicit-bool-cast");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<OperatorsRepresentationCheck>(
+        "readability-operators-representation");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");

docs/ReleaseNotes.rst

- Improved `cppcoreguidelines-no-malloc
 
   Allow custom memory management functions to be considered as well.
 
 - New `readability-misleading-indentation
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
 
   Finds misleading indentation where braces should be introduced or the code should be reformatted.
 
+- New `readability-operators-representation
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-operators-representation.html>`_ check
+
+  Flags (and replaces) the alternative tokens for binary and unary operators,
+  such as ``not`` (for ``!``) and ``or`` (for ``||``).
+
 - Added `ParameterThreshold` to `readability-function-size`.
 
   Finds functions that have more then `ParameterThreshold` parameters and emits a warning.
 
 - New `safety-no-assembler
   <http://clang.llvm.org/extra/clang-tidy/checks/safety-no-assembler.html>`_ check
 
   Finds uses of inline assembler.

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-misleading-indentation
    readability-misplaced-array-index
    readability-named-parameter
+   readability-operators-representation
    readability-non-const-parameter
    readability-redundant-control-flow
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release

docs/clang-tidy/checks/readability-operators-representation.rst

+.. title:: clang-tidy - readability-operators-representation
+
+readability-operators-representation
+====================================
+
+Flags (and replaces) the alternative tokens for binary and unary operators by
+their primary ones for consistency.
+
+======= ===========
+Primary Alternative
+======= ===========
+``&&``  ``and``
+``&=``  ``and_eq``
+``&``   ``bitand``
+``|``   ``bitor``
+``~``   ``compl``
+``!``   ``not``
+``!=``  ``not_eq``
+``||``  ``or``
+``|=``  ``or_eq``
+``^``   ``xor``
+``^=``  ``xor_eq``
+======= ===========

test/clang-tidy/readability-operators-representation.cpp

+// RUN: %check_clang_tidy %s readability-operators-representation %t
+
+void f() {
+  bool a, b, c;
+
+  c = a and b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: 'and' is an alternative token spelling; consider using '&&' [readability-operators-representation]
+  // CHECK-FIXES: c = a && b;
+  c and_eq a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'and_eq' is an alternative
+  // CHECK-FIXES: c &= a;
+  c = a bitand b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: 'bitand' is an alternative
+  // CHECK-FIXES: c = a & b;
+  c = a bitor b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: 'bitor' is an alternative
+  // CHECK-FIXES: c = a | b;
+  c = compl a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'compl' is an alternative
+  // CHECK-FIXES: c = ~ a;
+  c = not a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'not' is an alternative
+  // CHECK-FIXES: c = ! a;
+  c = a not_eq b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: 'not_eq' is an alternative
+  // CHECK-FIXES: c = a != b;
+  c = a or b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: 'or' is an alternative
+  // CHECK-FIXES: c = a || b;
+  c or_eq a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'or_eq' is an alternative
+  // CHECK-FIXES: c |= a;
+  c = a xor b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: 'xor' is an alternative
+  // CHECK-FIXES: c = a ^ b;
+  c xor_eq a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'xor_eq' is an alternative
+  // CHECK-FIXES: c ^= a;
+
+#define M a xor
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 'xor' is an alternative
+  // CHECK-FIXES: #define M a ^
+  c = M b;
+
+  int arr[2];
+  for (int i : arr) // OK (Here is an implicit != operator.)
+    ;
+
+  auto ptr = &c; // OK
+  auto i = -1;   // OK
+  c = a && b;    // OK
+  c &= a;        // OK
+  c = !a;        // OK
+}
+
+struct S {
+  friend S& operator and(const S &, const S &);
+};
+
+int g() {
+  S s1, s2;
+  S s3 = s1 and s2; // OK
+}