diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -43,6 +43,7 @@ SuspiciousCallArgumentCheck.cpp UniqueptrDeleteReleaseCheck.cpp UppercaseLiteralSuffixCheck.cpp + UseAlternativeTokensCheck.cpp UseAnyOfAllOfCheck.cpp LINK_LIBS diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -46,6 +46,7 @@ #include "SuspiciousCallArgumentCheck.h" #include "UniqueptrDeleteReleaseCheck.h" #include "UppercaseLiteralSuffixCheck.h" +#include "UseAlternativeTokensCheck.h" #include "UseAnyOfAllOfCheck.h" namespace clang { @@ -129,6 +130,8 @@ "readability-uniqueptr-delete-release"); CheckFactories.registerCheck( "readability-uppercase-literal-suffix"); + CheckFactories.registerCheck( + "readability-use-alternative-tokens"); CheckFactories.registerCheck( "readability-use-anyofallof"); } diff --git a/clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h b/clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h @@ -0,0 +1,46 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEALTERNATIVETOKENSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEALTERNATIVETOKENSCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/IncludeInserter.h" +#include "clang/Basic/SourceManager.h" + +namespace clang { +namespace tidy { +namespace readability { +/// Flags uses of symbol-based bitwise and logical operators. +class UseAlternativeTokensCheck : public ClangTidyCheck { +public: + using ClangTidyCheck::ClangTidyCheck; + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override { + this->SM = &SM; + } + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const SourceManager *SM; + + void checkSpelling(const UnaryOperator &); + void checkSpelling(const BinaryOperator &); + void checkSpelling(const CXXOperatorCallExpr &); +}; +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEALTERNATIVETOKENSCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp @@ -0,0 +1,159 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "UseAlternativeTokensCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/OperatorKinds.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Lex/Lexer.h" + +#include + +namespace clang { +namespace tidy { +namespace readability { +namespace { +AST_MATCHER(UnaryOperator, isBitwiseOrLogicalUnaryOp) { + return Node.getOpcode() == UnaryOperator::Opcode::UO_Not || + Node.getOpcode() == UnaryOperator::Opcode::UO_LNot; +} +} // namespace + +using ast_matchers::binaryOperation; +using ast_matchers::binaryOperator; +using ast_matchers::hasAnyOperatorName; +using ast_matchers::MatchFinder; +using ast_matchers::unaryOperator; + +void UseAlternativeTokensCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(unaryOperator(isBitwiseOrLogicalUnaryOp()).bind("unary"), + this); + // Finder->addMatcher(binaryOperator(isBitwiseOrLogicalBinaryOp()).bind("binary_builtin"), + // this); + Finder->addMatcher( + binaryOperation(hasAnyOperatorName("&&", "||", "!", "&", "|", "~", "^")) + .bind("operator"), + this); +} + +void UseAlternativeTokensCheck::checkSpelling(const UnaryOperator &Op) { + SourceLocation Loc = Op.getOperatorLoc(); + char First = *(SM->getCharacterData(Loc)); + if (std::isalpha(First)) + return; + + bool IsLogicalNot = Op.getOpcode() == UnaryOperator::Opcode::UO_LNot; + auto Hint = FixItHint::CreateReplacement(Op.getOperatorLoc(), + IsLogicalNot ? "not " : "compl "); + + diag(Loc, "use '%select{not|compl}0' for %select{logical|bitwise}0 negation") + << (IsLogicalNot ? 0 : 1) << Hint; +} + +void UseAlternativeTokensCheck::checkSpelling(const BinaryOperator &Op) { + SourceLocation Loc = Op.getOperatorLoc(); + char First = *(SM->getCharacterData(Loc)); + if (std::isalpha(First)) + return; + + using Opcode = BinaryOperator::Opcode; + Opcode OC = Op.getOpcode(); + switch (OC) { + case Opcode::BO_LAnd: + diag(Loc, "use 'and' for logical conjunctions") + << FixItHint::CreateReplacement(Loc, " and "); + break; + case Opcode::BO_And: + diag(Loc, "use 'bitand' for bitwise conjunctions") + << FixItHint::CreateReplacement(Loc, " bitand "); + break; + case Opcode::BO_Or: + diag(Loc, "use 'bitor' for bitwise disjunctions") + << FixItHint::CreateReplacement(Loc, " bitor "); + break; + case Opcode::BO_LOr: + diag(Loc, "use 'or' for logical disjunctions") + << FixItHint::CreateReplacement(Loc, " or "); + break; + case Opcode::BO_Xor: + diag(Loc, "use 'xor' for exclusive or") + << FixItHint::CreateReplacement(Loc, " xor "); + break; + default: + break; + } +} + +void UseAlternativeTokensCheck::checkSpelling(const CXXOperatorCallExpr &Op) { + SourceLocation Loc = Op.getOperatorLoc(); + char First = *(SM->getCharacterData(Loc)); + if (std::isalpha(First)) + return; + + using Opcode = OverloadedOperatorKind; + 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; + + Opcode OC = Op.getOperator(); + switch (OC) { + case And: + diag(Loc, "use 'and' for logical conjunctions") + << FixItHint::CreateReplacement(Loc, " and "); + break; + case Bitand: + diag(Loc, "use 'bitand' for bitwise conjunctions") + << FixItHint::CreateReplacement(Loc, " bitand "); + break; + case Bitor: + diag(Loc, "use 'bitor' for bitwise disjunctions") + << FixItHint::CreateReplacement(Loc, " bitor "); + break; + case Compl: + diag(Loc, "use 'compl' for bitwise negation") + << FixItHint::CreateReplacement(Loc, "compl "); + break; + case Not: + diag(Loc, "use 'not' for logical negation") + << FixItHint::CreateReplacement(Loc, "not "); + break; + case Or: + diag(Loc, "use 'or' for logical disjunctions") + << FixItHint::CreateReplacement(Loc, " or "); + break; + case Xor: + diag(Loc, "use 'xor' for exclusive or") + << FixItHint::CreateReplacement(Loc, " xor "); + break; + default: + break; + } +} + +void UseAlternativeTokensCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Op = Result.Nodes.getNodeAs("unary")) + return checkSpelling(*Op); + + if (const auto *Op = Result.Nodes.getNodeAs("operator")) + return checkSpelling(*Op); + + if (const auto *Op = Result.Nodes.getNodeAs("operator")) + return checkSpelling(*Op); +} +} // namespace readability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst @@ -0,0 +1,70 @@ +.. title:: clang-tidy - readability-use-alternative-tokens + +readability-use-alternative-tokens +================================== + +Finds uses of symbol-based logical and bitwise operators and recommends using +alternative tokens instead. + +Although symbols are the mainstream tokens in syntaxes derived from C, spelling +out the operators can be clearer. It more clearly expresses programmer intention +by reducing barriers for interpretation, makes small operators stand out, makes +similar tokens more visibly distinguishable, and reduces the chances for typos. + +.. code-block:: c++ + + // warning: use 'and' for logical conjunctions + x && y + + // warning: use 'bitand' for bitwise conjunctions + x & y + + // warning: use 'bitor' for bitwise disjunctions + x | y + + // warning: use 'compl' for bitwise negation + ~x + + // warning: use 'not' for logical negation + not x + + // warning: use 'or' for logical disjunctions + x || y + + // warning: use 'xor' for exclusive or + x ^ y + + +Limitations +=========== + +The following limitations are planned to be fixed within a reasonable timeframe. + +Language support +---------------- + +This check only supports C++ source at present. Once preprocessor support is +added, this check should be usable in C99 through C2X as well. + +Program composition +------------------- + +This check doesn't yet account for program composition. This means that the +warning is currently in conflict with piping C++20 ranges, as well as any other +library that uses ``|`` as a composition operator. + +.. code-block:: c++ + + // warning: use 'bitor' for logical disjunctions + auto evens = std::views::iota(0, 1'000) + | std::views::filter([](int const x) { return x % 2 == 0; }); + +The reason for this limitation is because it's not yet clear how to specify to +clang-tidy that an ``|`` is being used for composition over a bitwise +disjunction. Two ideas under consideration are: + +1. Have implementers apply an attribute such as ``[[clang::composable]]`` which + signals to the linter that ``|`` is preferred. + +2. Have clang-tidy users supply a list of types that can be considered as + composable. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-use-alternative-tokens.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-use-alternative-tokens.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-use-alternative-tokens.cpp @@ -0,0 +1,90 @@ +// RUN: %check_clang_tidy %s readability-use-alternative-tokens %t -- -- -I%S/Inputs + +// A note to the reader: the whitespace in this file is important: `true&&false` +// is lexically three tokens, but `trueandfalse` is lexed as a single +// identifier. This test needs to make sure that the fixit applies whitespace in +// its change. + +// clang-format off + +void f1() +{ + (void)(true and true); // no warning + (void)(true&&false); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use 'and' for logical conjunctions + // CHECK-FIXES: true and false + + (void)(1 bitand 2); // no warning + (void)(0&1); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions + // CHECK-FIXES: 0 bitand 1 + + (void)(1 bitor 2); // no warning + (void)(0|1); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions + // CHECK-FIXES: 0 bitor 1 + + (void)(compl 1); // no warning + (void)(~0); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation + // CHECK-FIXES: compl 0 + + (void)(not true); + (void)(!false); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation + // CHECK-FIXES: not false + + (void)(true or true); + (void)(true||false); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use 'or' for logical disjunctions + // CHECK-FIXES: true or false + + (void)(1 xor 0); + (void)(0^1); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or + // CHECK-FIXES: 0 xor 1 +} + +struct S { + S operator and(S) const; + S operator bitand(S) const; + S operator bitor(S) const; + S operator compl() const; + S operator not() const; + S operator or(S) const; + S operator xor(S) const; +}; + +void f5() +{ + S x; + S y; + + (void)(x&&y); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'and' for logical conjunctions + // CHECK-FIXES: x and y + + (void)(x&y); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions + // CHECK-FIXES: x bitand y + + (void)(x|y); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions + // CHECK-FIXES: x bitor y + + (void)(~x); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation + // CHECK-FIXES: compl x + + (void)(!x); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation + // CHECK-FIXES: not x + + (void)(x||y); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'or' for logical disjunctions + // CHECK-FIXES: x or y + + (void)(x^y); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or + // CHECK-FIXES: x xor y +} diff --git a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn --- a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn +++ b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn @@ -51,6 +51,7 @@ "SuspiciousCallArgumentCheck.cpp", "UniqueptrDeleteReleaseCheck.cpp", "UppercaseLiteralSuffixCheck.cpp", + "UseAlternativeTokensCheck.cpp", "UseAnyOfAllOfCheck.cpp", ] }