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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:133-134
         "readability-uppercase-literal-suffix");
+    CheckFactories.registerCheck<UseAlternativeTokensCheck>(
+        "readability-use-alternative-tokens");
     CheckFactories.registerCheck<UseAnyOfAllOfCheck>(
----------------
aaron.ballman wrote:
> 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.)
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.


================
Comment at: clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp:41-42
+  Finder->addMatcher(
+      binaryOperation(hasAnyOperatorName("&&", "||", "!", "&", "|", "~", "^"))
+          .bind("operator"),
+      this);
----------------
Quuxplusone wrote:
> `~` and `!` are not binary operations, right?
This confused me too. `binaryOperation` also takes into account all overloaded ops.


================
Comment at: clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.cpp:53-54
+  bool IsLogicalNot = Op.getOpcode() == UnaryOperator::Opcode::UO_LNot;
+  auto Hint = FixItHint::CreateReplacement(Op.getOperatorLoc(),
+                                           IsLogicalNot ? "not " : "compl ");
+
----------------
aaron.ballman wrote:
> 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
Thanks! This was one of the blockers for getting me C support.


================
Comment at: clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h:25
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
----------------
aaron.ballman wrote:
> We can support C for this as well, can't we? iso646.h has been around since C99.
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?


================
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.
+
----------------
Quuxplusone wrote:
> 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 `& | ^ ~ << >> &= |= ~= <<= >>=`.
As agreed with Aaron above, I'll be making this configurable at a later date.


================
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
----------------
Quuxplusone wrote:
> "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.
I think "program composition" is used enough in contemporary programming for readers to understand the text as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294