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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp:41-42
+  Finder->addMatcher(
+      binaryOperation(hasAnyOperatorName("&&", "||", "!", "&", "|", "~", "^"))
+          .bind("operator"),
+      this);
----------------
`~` and `!` are not binary operations, right?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst:6-7
+
+Finds uses of symbol-based logical and bitwise operators and recommends using
+alternative tokens instead.
+
----------------
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 `& | ^ ~ << >> &= |= ~= <<= >>=`.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst:49-52
+Program composition
+-------------------
+
+This check doesn't yet account for program composition. This means that the
----------------
"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.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-use-alternative-tokens.rst:58
+
+  // warning: use 'bitor' for logical disjunctions
+  auto evens = std::views::iota(0, 1'000)
----------------
s/logical/bitwise/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107294/new/

https://reviews.llvm.org/D107294