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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:61
+FixItHint AlternativeTokensCheck::createReplacement(SourceLocation Loc,
+                                                    StringRef S, int N) const {
+  // Only insert spaces if there aren't already spaces between operators
----------------
What's `N`? It's not immediately apparent for me from the callsites of this function.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:70-71
+void AlternativeTokensCheck::checkSpelling(const UnaryOperator &Op) {
+  SourceLocation Loc = Op.getOperatorLoc();
+  char First = *(SM->getCharacterData(Loc));
+  if (std::isalpha(First) || Loc.isMacroID())
----------------
Are we sure these will never return an invalid Loc, or dereference a null? Maybe it'd be worth to investigate, add an assertion (to help the people who might run static analysis on LLVM, if for nothing else), or an early return.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:90-91
+void AlternativeTokensCheck::checkSpelling(const BinaryOperator &Op) {
+  SourceLocation Loc = Op.getOperatorLoc();
+  char First = *(SM->getCharacterData(Loc));
+  if (std::isalpha(First) || Loc.isMacroID())
----------------
Same comment as in the `UnaryOperator` case.


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:124-125
+void AlternativeTokensCheck::checkSpelling(const CXXOperatorCallExpr &Op) {
+  SourceLocation Loc = Op.getOperatorLoc();
+  char First = *(SM->getCharacterData(Loc));
+  if (std::isalpha(First) || Loc.isMacroID())
----------------
Ditto. (Usually there might be issues if the location is coming from generated code or templates or macros or something... I fear this could be mostly prevalent in the user-defined operator case, i.e. this method.)


================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h:20-21
+namespace readability {
+/// Flags uses of symbol-based bitwise and logical operators.
+class AlternativeTokensCheck : public ClangTidyCheck {
+public:
----------------
Following from gone thread due to file rename.

>>! In D107294#2923102, @cjdb wrote:
> Not sure I'm following you here: are you suggesting I put the contents of my `rst` file in a comment here?

Not the entire //RST//, but the one-sentence or first-paragraph "pitch". For example, let's see `bugprone-branch-clone`'s class's doc-comment:

```
/// A check for detecting if/else if/else chains where two or more branches are
/// Type I clones of each other (that is, they contain identical code), for
/// detecting switch statements where two or more consecutive branches are
/// Type I clones of each other, and for detecting conditional operators where
/// the true and false expressions are Type I clones of each other.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-branch-clone.html
class BranchCloneCheck : public ClangTidyCheck {
```

Or another one selected randomly, `performance-no-automatic-move`:

```
/// Finds local variables that cannot be automatically moved due to constness.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/performance-no-automatic-move.html
class NoAutomaticMoveCheck : public ClangTidyCheck {
```

So there is a one-paragraph summary of the check itself (it could be shorter than here...), and there is a text that's generated from a template (I think `add-new-check.py` sets the new check's files as such when you run it), which basically just links the upstream official website render of your check's documentation HTML.


================
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>(
----------------
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.)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-alternative-tokens.rst:6-7
+
+Finds uses of symbol-based logical and bitwise operators and recommends using
+alternative tokens instead.
+
----------------
I think (for now, until the check is improved/extended) this first paragraph / oneliner added to the doccomment in the header shall suffice.

It's only to ensure that the automatically generated documentation for the check's class has a comment from which documentation readers can look into what the check actually does (from a user's standpoint), by reaching this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107294