[clang-tidy] adds warning to suggest users replace symbols with words

[PiotrZSL]

Summary

C++'s alternative tokens are a good way to improve readability and
minimise bugs by expressing intention using words instead of symbols
(which can both be more easily misspelt _and_ misread).

This warning analyses the binary logical and bitwise operators and warns
if they aren't spelt using the alternative tokens. Bitwise compound
assignment and != operators are not in scope since there's nothing to
misspel or misread.

Future work:

• add a way for the compiler to detect when the pipe operator (|) is not being used for a bitwise-or operation
• add preprocessor support
• add config options

[aaron.ballman]

The usual community viewpoint on off-by-default diagnostics is that they're generally low value and not something that users will manually enable. Because I don't think this will ever be reasonable to enable by default due to it inherently being a matter of style, I think this functionality is better handled by clang-tidy as a check in the readability module. I would note that it should also be supported in C (C99 and later) by also inserting #include <iso646.h> using the usual fix-it utilities.

[cjdb]

In D107294#2920355, @aaron.ballman wrote:

The usual community viewpoint on off-by-default diagnostics is that they're generally low value and not something that users will manually enable.

I was under the impression many warnings weren't enabled by default?

I think this functionality is better handled by clang-tidy

Ack. I've not touched clang-tidy yet; how different do you expect the code to look to what I've got right now?

[aaron.ballman]

In D107294#2920477, @cjdb wrote:

In D107294#2920355, @aaron.ballman wrote:

The usual community viewpoint on off-by-default diagnostics is that they're generally low value and not something that users will manually enable.

I was under the impression many warnings weren't enabled by default?

We certainly have some! My understanding is that many of those were added before we realized most people don't actually enable off-by-default warnings, so it's only "recently" (within the past 5-10 years) that we've pushed back more consistently on off-by-default diagnostics (and even then, we still allow them sometimes; -Wfallthrough is a good example of that).

I think this functionality is better handled by clang-tidy

Ack. I've not touched clang-tidy yet; how different do you expect the code to look to what I've got right now?

Because you're doing this at the AST node level, I would expect it to be pretty easy. If you want to do it within preprocessor expressions (or other places where the information isn't in the AST node), it's still doable but might be a bit more complex.

[cjdb]

moves patch from Clang to clang-tidy

[whisperity]

clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h

5–6 ↗

(On Diff #363638)

This seems to be very old code, there was a licence change approx. 2 years ago.

20–22 ↗

(On Diff #363638)

But also there is a convention for the documentation comment for the main class of checks, and the code here should adhere to that.

clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst

14 ↗

(On Diff #363638)

What does this refer to? Perhaps a code example and a clearly indicated Limitations heading would be more clear to indicate whether someone would want to enable this check.

21–27 ↗

(On Diff #363638)

Either use plural xor singular for all of these printouts, to keep consistency.

[cjdb]

clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h

5–6 ↗

(On Diff #363638)

Yikes, that's what I get for not reading what I'm copying. Should there be an NFC to go and update the other checks' licences too?

20–22 ↗

(On Diff #363638)

Not sure I'm following you here: are you suggesting I put the contents of my rst file in a comment here?

clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst

21–27 ↗

(On Diff #363638)

negation is the plural in this case.

[cjdb]

• updates licences
• updates documentation

[cjdb]

removes obsolete code

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

135–136

I think this might be a case where we want the check to either recommend using alternative tokens or recommend against using alternative tokens via a configuration option (so users can control readability in either direction). If you agree that's a reasonable design, then I'd recommend we name this readability-alternative-tokens to be a bit more generic. (Note, we can always do the "don't use alternative tokens" implementation in a follow-up patch if you don't want to do that work immediately.)

clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp

53–54 ↗

(On Diff #363850)

For C code, you could augment this FixIt with an include inserter, e.g., https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp#L112

clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h

5–6 ↗

(On Diff #363638)

Should there be an NFC to go and update the other checks' licences too?

I think that'd be useful!

25 ↗

(On Diff #363850)

We can support C for this as well, can't we? iso646.h has been around since C99.

[• Quuxplusone]

clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp

41–42 ↗

(On Diff #363850)

~ and ! are not binary operations, right?

clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst

6–7 ↗

(On Diff #363850)

I strongly recommend splitting this check in two parts:

• Use and, or, not for logical operations.
• Use bitand, bitor, xor, compl for non-logical (bitwise) operations.

The reason I recommend this is that I have seen people (never in a real codebase, mind you, but at least bloggers and such) recommend and/or/not for logical operations; this is a long-standing convention in other languages such as Python and Perl. Whereas I have never seen anyone recommend e.g. (x bitand compl mask) over (x & ~mask). So coupling the two ideas together seems counterproductive, because even if someone wanted to use the Python style in their codebase, they wouldn't be able to enforce it using this check, because this check would be full of false positives related to the bitwise operators.
The current check's recommendation to replace (a || b) => (a or b), (a ^ b) => (a xor b) is particularly pernicious to non-language-lawyers, who might assume that or and xor represented the same "part of speech" — either both bitwise or both logical. I think this is a major motivation for the Pythonic style: use English for logical and or not, and use symbols for mathematical & | ^ ~ << >> &= |= ~= <<= >>=.

49–52 ↗

(On Diff #363850)

"Program composition" is not a term of art (except in the extremely general sense of composing programs, i.e. programming). I recommend retitling this section "Use of | as a pipe" or "Use of | with C++20 Ranges".

It might be worth adding a brief mention that the same is true of any minigame/DSL embedded within C++; for example, this clang-tidy check will also recommend changes like

qi::rule<std::string::iterator, boost::variant<int, bool>(),
    ascii::space_type> value = qi::int_ | qi::bool_;
                                        ^
warning: use 'bitor' for bitwise disjunctions

and

std::fstream file("hello.txt", std::ios::in | std::ios::out);
                                            ^
warning: use 'bitor' for bitwise disjunctions

which are probably not desirable for the user-programmer.

58 ↗

(On Diff #363850)

s/logical/bitwise/

[cjdb]

• renames check to 'readability-alternative-tokens'
• adds section in documentation about configurability
• adds support for C source
• corrects whitespace handling in fixits

[cjdb]

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

135–136

Hrmm.... I'll do the rename now, but this might be better off as a later patch. I'd rather focus on getting what I have right (along with my teased extensions) and then work on the opposite direction. That will (probably) be an easy slip-in.

clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp

41–42 ↗

(On Diff #363850)

This confused me too. binaryOperation also takes into account all overloaded ops.

53–54 ↗

(On Diff #363850)

Thanks! This was one of the blockers for getting me C support.

clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h

25 ↗

(On Diff #363850)

I was having difficulty in my C test for this, wimped out, and put it into the "I'll do it later" basket. Lemme double check that's the case?

clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst

6–7 ↗

(On Diff #363850)

As agreed with Aaron above, I'll be making this configurable at a later date.

49–52 ↗

(On Diff #363850)

I think "program composition" is used enough in contemporary programming for readers to understand the text as it is.

[whisperity]

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp

61

What's N? It's not immediately apparent for me from the callsites of this function.

70–71

Are we sure these will never return an invalid Loc, or dereference a null? Maybe it'd be worth to investigate, add an assertion (to help the people who might run static analysis on LLVM, if for nothing else), or an early return.

90–91

Same comment as in the UnaryOperator case.

124–125

Ditto. (Usually there might be issues if the location is coming from generated code or templates or macros or something... I fear this could be mostly prevalent in the user-defined operator case, i.e. this method.)

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h

20–21

Following from gone thread due to file rename.

In D107294#2923102, @cjdb wrote:

Not sure I'm following you here: are you suggesting I put the contents of my rst file in a comment here?

Not the entire RST, but the one-sentence or first-paragraph "pitch". For example, let's see bugprone-branch-clone's class's doc-comment:

/// A check for detecting if/else if/else chains where two or more branches are
/// Type I clones of each other (that is, they contain identical code), for
/// detecting switch statements where two or more consecutive branches are
/// Type I clones of each other, and for detecting conditional operators where
/// the true and false expressions are Type I clones of each other.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-branch-clone.html
class BranchCloneCheck : public ClangTidyCheck {

Or another one selected randomly, performance-no-automatic-move:

/// Finds local variables that cannot be automatically moved due to constness.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/performance-no-automatic-move.html
class NoAutomaticMoveCheck : public ClangTidyCheck {

So there is a one-paragraph summary of the check itself (it could be shorter than here...), and there is a text that's generated from a template (I think add-new-check.py sets the new check's files as such when you run it), which basically just links the upstream official website render of your check's documentation HTML.

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

135–136

(As someone who's had checkers on review for multiple years I've no hard feelings about the scheduling.)

But please do add a few FIXME: or TODO: or IDEA: or some similar comments to the code somewhere about the suggested follow-ups. (Just so they don't go away when this review is closed.)

clang-tools-extra/docs/clang-tidy/checks/readability-alternative-tokens.rst

6–7

I think (for now, until the check is improved/extended) this first paragraph / oneliner added to the doccomment in the header shall suffice.

It's only to ensure that the automatically generated documentation for the check's class has a comment from which documentation readers can look into what the check actually does (from a user's standpoint), by reaching this file.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

135–136

Yeah, I wasn't trying to sign you up for the implementation effort, just making sure the name is somewhat more future-proofed. Adding a FIXME comment to the code to explain the potential next steps would be a good thing.

[cjdb]

• adds TODOs
• adds asserts
• removes "imported" SourceManager (apparently there was already a pointer in Result)
• fixes C++ documentation to match opening line of RST doc

[cjdb]

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h

20–21

Thanks for the comprehensive examples!

[cjdb]

Ping @aaron.ballman

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp

52–55

70

Hrmm, I feel like std::isspace() is not going to properly handle all the Unicode whitespace, but I don't think we handle them all anyway as I notice the lexer is using isHorizontalWhitespace(), isVerticalWhitespace(), and isWhitespace() from CharInfo.h. Maybe we should use the same utility here just to match the lexer? (I don't feel strongly, just tickled my spidey senses.)

79–80

Are you planning to do this as part of this patch (if not, should this switch to FIXME)?

86

isLetter()? (or potentially isIdentifierHead() if we need to care about dollar-sign prefixed identifiers)

105

Same here as above.

112

Same here as above.

118–121

I think these can be replaced with a helper function along these lines:

void emitDiag(const SourceManager &SM, SourceLocation Loc, StringRef Op, StringRef OpDesc, int Lookahead) {
  diag(Loc, ("use '" + Op + "' for " + OpDesc).str()) <<
    createReplacement(SM, Loc, Op, Lookahead) << includeIso646(SM, Loc);
}

WDYT?

145–149

FIXMEs?

159–165

Nit: given that these are only used once, I'd rather just spell out the case labels longhand.

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h

36

clang-tools-extra/clang-tidy/readability/StrictConstCorrectness.h

9 ↗

(On Diff #364555)

Did you mean to include this file in this review?

clang-tools-extra/clang-tidy/readability/StrictConstCorrectness.kpp

1 ↗

(On Diff #364555)

Same for this one?

clang-tools-extra/docs/clang-tidy/checks/readability-alternative-tokens.rst

5

clang/test/Analysis/diagnostics/readability-strict-const-correctness.cpp

1 ↗

(On Diff #364555)

No idea how this managed to trigger a CI failure of line 1: fg: no job control :-D

[cor3ntin]

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp

70

I just saw this comment randomly in my mail.
I agree we should not use isspace - it's also probably less efficient because of locale. And we don't want lexing to depend on the host locale.
And makes it easier to extend the set of supported whitespace if we ever do thatt

[cjdb]

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp

86

Is this necessary in the given context? The reason I was confident in using isalpha is because we know this is an operator and the only values that this could possibly be are one of a, b, c, n, o, or x.

(cc @cor3ntin)

clang-tools-extra/clang-tidy/readability/StrictConstCorrectness.h

9 ↗

(On Diff #364555)

Lol nope. My bad.

clang/test/Analysis/diagnostics/readability-strict-const-correctness.cpp

1 ↗

(On Diff #364555)

How did these files even get into this branch!? Sorry for the noise.

[cor3ntin]

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp

86

Definitively.
Both because trying to make assumption based on the current set of leading symbols in alternative token seems brittle to me.
But mostly because isalpha answers "is this byte interpreted in the encoding associated to the current locale a letter". in some scenario (clang running on zOS for example) this will never give the right answer,

I agree with Aaron: use isIdentifierHead()

But maybe I'd go further: Have you considered first checking for the opcode first, then the spelling?

[PiotrZSL]

Obsolete by D144522

[njames93]

The for Pipe/BitwiseOr issue. One heuristic could be:

• If it has any kind of template dependence don't diagnost
• If its a BinaryOperator, its safe to diagnose.
• If its a CxxOperatorCallExpr, the simplest case I can think of is to check the spelling of the operator (operator| vs operator bitor)

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp

70

Is it not just wiser to always add the spaces and just let clang-format do its thing.

118–121

diag("use '%0' for %1") << Op << OpDesc << Fixes;

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h

+//===----------------------------------------------------------------------===//
+//
+// 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/Diagnostic.h"
+#include "clang/Basic/SourceManager.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+/// Flags uses of symbol-based bitwise and logical operators.
+class AlternativeTokensCheck : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.C99 || LangOpts.CPlusPlus;
+  }
+
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override {
+    this->SM = &SM;
+    Includer.registerPreprocessor(PP);
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const SourceManager *SM = nullptr;
+  utils::IncludeInserter Includer{
+      Options.getLocalOrGlobal("IncludeStyle", utils::IncludeSorter::IS_LLVM)};
+
+  void checkSpelling(const UnaryOperator &Op);
+  void checkSpelling(const BinaryOperator &Op);
+  void checkSpelling(const CXXOperatorCallExpr &Op);
+
+  Optional<FixItHint> includeIso646(SourceLocation Loc);
+  FixItHint createReplacement(SourceLocation Loc, StringRef S, int N) const;
+  char lookahead(SourceLocation Loc, int N) const;
+};
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEALTERNATIVETOKENSCHECK_H

clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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 "AlternativeTokensCheck.h"
+#include "../utils/IncludeInserter.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 "clang/Lex/Preprocessor.h"
+
+#include <cstring>
+
+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::hasAnyOperatorName;
+using ast_matchers::MatchFinder;
+using ast_matchers::unaryOperator;
+
+void AlternativeTokensCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(unaryOperator(isBitwiseOrLogicalUnaryOp()).bind("unary"),
+                     this);
+  Finder->addMatcher(
+      binaryOperation(hasAnyOperatorName("&&", "||", "!", "&", "|", "~", "^"))
+          .bind("operator"),
+      this);
+}
+
+Optional<FixItHint> AlternativeTokensCheck::includeIso646(SourceLocation Loc) {
+  if (!getLangOpts().C99) {
+    return llvm::None;
+  }
+
+  return Includer.createIncludeInsertion(SM->getFileID(Loc), "<iso646.h>");
+}
+
+char AlternativeTokensCheck::lookahead(SourceLocation Loc, int N) const {
+  return *(SM->getCharacterData(Loc.getLocWithOffset(N)));
+}
+
+FixItHint AlternativeTokensCheck::createReplacement(SourceLocation Loc,
+                                                    StringRef S, int N) const {
+  // Only insert spaces if there aren't already spaces between operators
+  StringRef SpaceBefore = std::isspace(lookahead(Loc, -1)) ? "" : " ";
+  StringRef SpaceAfter = std::isspace(lookahead(Loc, N)) ? "" : " ";
+  return FixItHint::CreateReplacement(Loc,
+                                      (SpaceBefore + S + SpaceAfter).str());
+}
+
+void AlternativeTokensCheck::checkSpelling(const UnaryOperator &Op) {
+  SourceLocation Loc = Op.getOperatorLoc();
+  char First = *(SM->getCharacterData(Loc));
+  if (std::isalpha(First) || Loc.isMacroID())
+    return;
+
+  switch (Op.getOpcode()) {
+  case UnaryOperator::Opcode::UO_Not:
+    diag(Loc, "use 'compl' for bitwise negation")
+        << createReplacement(Loc, "compl", 1) << includeIso646(Loc);
+    return;
+  case UnaryOperator::Opcode::UO_LNot:
+    diag(Loc, "use 'not' for logical negation")
+        << createReplacement(Loc, "not", 1) << includeIso646(Loc);
+    return;
+  default:
+    return;
+  }
+}
+
+void AlternativeTokensCheck::checkSpelling(const BinaryOperator &Op) {
+  SourceLocation Loc = Op.getOperatorLoc();
+  char First = *(SM->getCharacterData(Loc));
+  if (std::isalpha(First) || Loc.isMacroID())
+    return;
+
+  using Opcode = BinaryOperator::Opcode;
+  Opcode OC = Op.getOpcode();
+  switch (OC) {
+  case Opcode::BO_LAnd:
+    diag(Loc, "use 'and' for logical conjunctions")
+        << createReplacement(Loc, "and", 2) << includeIso646(Loc);
+    break;
+  case Opcode::BO_And:
+    diag(Loc, "use 'bitand' for bitwise conjunctions")
+        << createReplacement(Loc, "bitand", 1) << includeIso646(Loc);
+    break;
+  case Opcode::BO_Or:
+    diag(Loc, "use 'bitor' for bitwise disjunctions")
+        << createReplacement(Loc, "bitor", 1) << includeIso646(Loc);
+    break;
+  case Opcode::BO_LOr:
+    diag(Loc, "use 'or' for logical disjunctions")
+        << createReplacement(Loc, "or", 2) << includeIso646(Loc);
+    break;
+  case Opcode::BO_Xor:
+    diag(Loc, "use 'xor' for exclusive or")
+        << createReplacement(Loc, "xor", 1) << includeIso646(Loc);
+    break;
+  default:
+    break;
+  }
+}
+
+void AlternativeTokensCheck::checkSpelling(const CXXOperatorCallExpr &Op) {
+  SourceLocation Loc = Op.getOperatorLoc();
+  char First = *(SM->getCharacterData(Loc));
+  if (std::isalpha(First) || Loc.isMacroID())
+    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")
+        << createReplacement(Loc, "and", 2) << includeIso646(Loc);
+    break;
+  case Bitand:
+    diag(Loc, "use 'bitand' for bitwise conjunctions")
+        << createReplacement(Loc, "bitand", 1) << includeIso646(Loc);
+    break;
+  case Bitor:
+    diag(Loc, "use 'bitor' for bitwise disjunctions")
+        << createReplacement(Loc, "bitor", 1) << includeIso646(Loc);
+    break;
+  case Compl:
+    diag(Loc, "use 'compl' for bitwise negation")
+        << createReplacement(Loc, "compl", 1) << includeIso646(Loc);
+    break;
+  case Not:
+    diag(Loc, "use 'not' for logical negation")
+        << createReplacement(Loc, "not", 1) << includeIso646(Loc);
+    break;
+  case Or:
+    diag(Loc, "use 'or' for logical disjunctions")
+        << createReplacement(Loc, "or", 2) << includeIso646(Loc);
+    break;
+  case Xor:
+    diag(Loc, "use 'xor' for exclusive or")
+        << createReplacement(Loc, "xor", 1) << includeIso646(Loc);
+    break;
+  default:
+    break;
+  }
+}
+
+void AlternativeTokensCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Op = Result.Nodes.getNodeAs<UnaryOperator>("unary"))
+    return checkSpelling(*Op);
+
+  if (const auto *Op = Result.Nodes.getNodeAs<BinaryOperator>("operator"))
+    return checkSpelling(*Op);
+
+  if (const auto *Op = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("operator"))
+    return checkSpelling(*Op);
+}
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyReadabilityModule
+  AlternativeTokensCheck.cpp
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ConvertMemberFunctionsToStatic.cpp
   DeleteNullPointerCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionCognitiveComplexityCheck.cpp

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 // 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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AlternativeTokensCheck.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionCognitiveComplexityCheck.h"

 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AlternativeTokensCheck>(
+        "readability-alternative-tokens");
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ConstReturnTypeCheck>(
         "readability-const-return-type");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SimplifyBooleanExprCheck>(
         "readability-simplify-boolean-expr");
     CheckFactories.registerCheck<SuspiciousCallArgumentCheck>(
         "readability-suspicious-call-argument");
     CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
         "readability-uniqueptr-delete-release");
     CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>(
         "readability-uppercase-literal-suffix");
     CheckFactories.registerCheck<UseAnyOfAllOfCheck>(
         "readability-use-anyofallof");
   }
 };
 
 // Register the ReadabilityModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ReadabilityModule>
     X("readability-module", "Adds readability-related checks.");
 
 } // namespace readability
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ReadabilityModule.
 volatile int ReadabilityModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/readability-alternative-tokens.rst

+.. title:: clang-tidy - readability-alternative-tokens
+
+readability-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
+
+Supported languages
+===================
+
+* C99 and later
+* C++98 and later
+
+Limitations
+===========
+
+The following limitations are planned to be fixed within a reasonable timeframe.
+
+Configurability
+---------------
+
+A later variant of this patch will support being able to configure how logical
+and bitwise operators are spelt.
+
+Preprocessor support
+--------------------
+
+This does not currently support the preprocessor.
+
+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 bitwise disjunctions
+  auto evens = std::views::iota(0, 1'000)
+             | std::views::filter([](int const x) { return x % 2 == 0; });
+
+  // warning: use 'bitor' for bitwise disjunctions
+  auto value = qi::int_ | qi::bool_;
+
+  // warning: use 'bitor' for bitwise disjunctions
+  std::fstream file("hello.txt", std::ios::in | std::ios::out);
+
+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 genuine 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.

clang-tools-extra/test/clang-tidy/checkers/readability-alternative-tokens-no-warn.c

+// RUN: %check_clang_tidy %s readability-alternative-tokens %t
+
+// A note to the reader: the whitespace in this file is important: `1&&0`
+// is lexically three tokens, but `1and0` is lexed as a single
+// identifier. This test needs to make sure that the fixit applies whitespace in
+// its change.
+
+#include <iso646.h>
+
+// clang-format off
+
+void f1()
+{
+  (void)(1 and 0);
+  (void)(0 bitand 1);
+  (void)(0 bitor 1);
+  (void)(compl 0);
+  (void)(not 0);
+  (void)(1 or 0);
+  (void)(0 xor 1);
+
+  // This is here because clang-tidy tests seemingly need at least on CHECK-MESSAGES.
+  (void)(0 && 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 0 and 1
+}

clang-tools-extra/test/clang-tidy/checkers/readability-alternative-tokens.c

+// RUN: %check_clang_tidy %s readability-alternative-tokens %t
+
+// A note to the reader: the whitespace in this file is important: `1&&0`
+// is lexically three tokens, but `1and0` is lexed as a single
+// identifier. This test needs to make sure that the fixit applies whitespace in
+// its change.
+
+// CHECK-FIXES: #include <iso646.h>
+
+// clang-format off
+
+void logical_and()
+{
+  (void)(1&&0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+
+  (void)(1&& 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+
+  (void)(1 &&0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+
+  (void)(1 && 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+}
+
+void bitwise_and()
+{
+  (void)(0&1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+
+  (void)(0 &1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+
+  (void)(0& 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+
+  (void)(0 & 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+}
+
+void bitwise_or() {
+  (void)(0|1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+
+  (void)(0| 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+
+  (void)(0 |1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+
+  (void)(0 | 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+}
+
+void bitwise_not() {
+  (void)(~0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation
+  // CHECK-FIXES: compl 0
+
+  (void)(~ 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation
+  // CHECK-FIXES: compl 0
+}
+
+void logical_not() {
+  (void)(!0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not 0
+
+  (void)(! 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not 0
+}
+
+void logical_or() {
+  (void)(1||0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+
+  (void)(1|| 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+
+  (void)(1 ||0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+
+  (void)(1 || 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+}
+
+void bitwise_xor() {
+  (void)(0^1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+
+  (void)(0^ 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+
+  (void)(0 ^1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+
+  (void)(0 ^ 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+}

clang-tools-extra/test/clang-tidy/checkers/readability-alternative-tokens.cpp

+// RUN: %check_clang_tidy %s readability-alternative-tokens %t
+
+// 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 logical_and()
+{
+  (void)(1&&0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+
+  (void)(1&& 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+
+  (void)(1 &&0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+
+  (void)(1 && 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: 1 and 0
+}
+
+void bitwise_and()
+{
+  (void)(0&1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+
+  (void)(0 &1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+
+  (void)(0& 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+
+  (void)(0 & 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitand' for bitwise conjunctions
+  // CHECK-FIXES: 0 bitand 1
+}
+
+void bitwise_or() {
+  (void)(0|1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+
+  (void)(0| 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+
+  (void)(0 |1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+
+  (void)(0 | 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: 0 bitor 1
+}
+
+void bitwise_not() {
+  (void)(~0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation
+  // CHECK-FIXES: compl 0
+
+  (void)(~ 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'compl' for bitwise negation
+  // CHECK-FIXES: compl 0
+}
+
+void logical_not() {
+  (void)(!0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not 0
+
+  (void)(! 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not 0
+}
+
+void logical_or() {
+  (void)(1||0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+
+  (void)(1|| 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+
+  (void)(1 ||0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+
+  (void)(1 || 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: 1 or 0
+}
+
+void bitwise_xor() {
+  (void)(0^1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+
+  (void)(0^ 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+
+  (void)(0 ^1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: 0 xor 1
+
+  (void)(0 ^ 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: 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 overloaded_and() {
+  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 'and' for logical conjunctions
+  // CHECK-FIXES: x and y
+
+  (void)(x &&y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'and' for logical conjunctions
+  // CHECK-FIXES: x and y
+
+  (void)(x && y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: 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 overloaded_bitand() {
+  S x;
+  S y;
+
+  (void)(x|y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: x bitor y
+
+  (void)(x| y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: x bitor y
+
+  (void)(x |y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: x bitor y
+
+  (void)(x | y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'bitor' for bitwise disjunctions
+  // CHECK-FIXES: x bitor y
+}
+
+void overloaded_compl() {
+  S x;
+  S 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 'compl' for bitwise negation
+  // CHECK-FIXES: compl x
+}
+
+void overloaded_not() {
+  S x;
+  S y;
+
+  (void)(!x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not x
+
+  (void)(! x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'not' for logical negation
+  // CHECK-FIXES: not x
+}
+
+void overloaded_or() {
+  S x;
+  S y;
+
+  (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 'or' for logical disjunctions
+  // CHECK-FIXES: x or y
+
+  (void)(x ||y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: x or y
+
+  (void)(x || y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'or' for logical disjunctions
+  // CHECK-FIXES: x or y
+}
+
+void overloaded_xor() {
+  S x;
+  S y;
+
+  (void)(x^y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: x xor y
+
+  (void)(x^ y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: x xor y
+
+  (void)(x ^y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: x xor y
+
+  (void)(x ^ y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'xor' for exclusive or
+  // CHECK-FIXES: x xor y
+}

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/readability/BUILD.gn

 static_library("readability") {
   output_name = "clangTidyReadabilityModule"
   configs += [ "//llvm/utils/gn/build:clang_code" ]
   deps = [
     "//clang-tools-extra/clang-tidy",
     "//clang-tools-extra/clang-tidy/utils",
     "//clang/lib/AST",
     "//clang/lib/ASTMatchers",
     "//clang/lib/Basic",
     "//clang/lib/Lex",
     "//clang/lib/Tooling",
     "//llvm/lib/Support",
   ]
   sources = [
+    "AlternativeTokensCheck.cpp",
     "AvoidConstParamsInDecls.cpp",
     "BracesAroundStatementsCheck.cpp",
     "ConstReturnTypeCheck.cpp",
     "ContainerSizeEmptyCheck.cpp",
     "ConvertMemberFunctionsToStatic.cpp",
     "DeleteNullPointerCheck.cpp",
     "ElseAfterReturnCheck.cpp",
     "FunctionCognitiveComplexityCheck.cpp",