aaron.ballman 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>(
----------------
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.)
================
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 ");
+
----------------
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
================
Comment at:
clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h:25
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
----------------
We can support C for this as well, can't we? iso646.h has been around since C99.
================
Comment at:
clang-tools-extra/clang-tidy/readability/UseAlternativeTokensCheck.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
cjdb wrote:
> whisperity wrote:
> > This seems to be very old code, there was a licence change approx. 2 years
> > ago.
> 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?
> Should there be an NFC to go and update the other checks' licences too?
I think that'd be useful!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107294/new/
https://reviews.llvm.org/D107294
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
