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

[Aaron Ballman via Phabricator via cfe-commits]

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