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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
