[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>(
----------------
whisperity wrote:
> cjdb wrote:
> > 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.
> (As someone who's had checkers on review for multiple years I've no hard feelings about the scheduling.)
> 
> But please do add a few `FIXME:` or `TODO:` or `IDEA:` or some similar comments to the code somewhere about the suggested follow-ups. (Just so they don't go away when this review is closed.)
Yeah, I wasn't trying to sign you up for the implementation effort, just making sure the name is somewhat more future-proofed. Adding a FIXME comment to the code to explain the potential next steps would be a good thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294