[PATCH] D144522: [clang-tidy] Add readability-operators-representation check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Looks really good, thank you! I have only very minor comments, mostly style nits and suggestions for improved readability.



================
Comment at: clang-tools-extra/clang-tidy/readability/OperatorsRepresentationCheck.cpp:25
+
+StringRef getOperatorSpelling(SourceLocation Loc, ASTContext &Context) {
+  if (Loc.isInvalid())
----------------
As per LLVM guidelines this should be static, outside the anonymous namespace.


================
Comment at: clang-tools-extra/clang-tidy/readability/OperatorsRepresentationCheck.cpp:176
+
+  if (isAnyOperatorEnabled(BinaryOperators, OperatorsRepresentation)) {
+    Finder->addMatcher(
----------------
Nit: would be good to add a one-line comment before each of the code blocks to describe at high-level what they do, to get a better birds-eye view of the code structure in this large function. At first sight it took me a while to spot the difference and understand why it's not exactly duplicated code.

Alternatively creating a function for each of the 3 addMatcher lines could be self-documenting and make the "registerMatches" smaller.


================
Comment at: clang-tools-extra/clang-tidy/readability/OperatorsRepresentationCheck.cpp:202
+                    getRepresentation(BinaryOperators, "^=", "xor_eq"))))
+            .bind("bo"),
+        this);
----------------
Nit: could get a bit more descriptive name, like "binary_op"


================
Comment at: clang-tools-extra/clang-tidy/readability/OperatorsRepresentationCheck.cpp:220
+  if (!isAnyOperatorEnabled(OverloadedOperators, OperatorsRepresentation) &&
+      isAnyOperatorEnabled(OverloadedOperators, UnaryRepresentation))
+    return;
----------------
Is this condition intended to be negated too? Also, would it make sense to move this early return to the beginning of the function?


================
Comment at: clang-tools-extra/clang-tidy/readability/OperatorsRepresentationCheck.cpp:227
+          anyOf(
+              hasInvalidRepresentationO(
+                  OO_AmpAmp,
----------------
Nit: would be good to spell this out for better readability.


================
Comment at: clang-tools-extra/clang-tidy/readability/OperatorsRepresentationCheck.h:13
+#include "../ClangTidyCheck.h"
+#include "llvm/ADT/SmallVector.h"
+#include <vector>
----------------
 I believe this include is not in use in this header?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability/operators-representation.rst:159
+    Configuration can be mixed, for example: `and;||;not`.
+    The default value is `and;or;not`.
+
----------------
PiotrZSL wrote:
> carlosgalvezp wrote:
> > It's unclear what the default value is for the other operators. From the code I see that ATR is the default - in that case I think the documentation should either a) provide the full list of defaults here, or b) simply mention "ATR style is the default". As a user I would prefer the former, so I know what keywords I am allowed to type in here, without having to go and consult cppreference.
> > 
> > Actually, now that I think about it, wouldn't it be better to implement this as an enum, similar to the readability-identifier-naming checks? Is there a use case for having `and;||;not`, i.e. a mix of style within the same category (e.g. BinaryOperators)?
> > 
> > In that case, it could look something like:
> > 
> > ```
> > readability-operators-representation.BinaryOperatorsStyle = Traditional
> > readability-operators-representation.BinaryOperatorsStyle = Alternative
> > ```
> > 
> > This would be easier to use from a user perspective, and IMHO would lead to a more consistent codebase, where one style or the other is chosen for all operators in each category.
> I choose this style just because with single option you can define whatever you like.
> You can easily be less or more strict, enforce for example alternative tokens for some, and enforce traditional for other.
> 
> Entire section "Alternative Token Representation" is here so you wouldn't need to go to cppreference, there is an mapping.
> 
> And note that default config don't enforce alternative tokens for all, just for 3 most common.
> Not everyone know about alternative token existence. And cppreference don't describe why they exist. This is why this description with examples were added, about pros and some cons. Not only engineers read checks documentations , quality/product managers do that also.
> And most of checks descriptions basically don't provide any information that would answer a question: "Why I should enable this check, what I will gain".
> 
> I agree, that some additional paragraph about example configurations (less, more strict) and maybe some clarification about options could be added. And some clarification why options are split into two with example, I will think about this.
> 
> As for "Alternative Token Representation" if you wan't I can extract from it also "Traditional Token Representation" and put there pros and cons. And move mapping table in front of both of them.
> 
Really appreciate the changes to the documentation, as a user it's much more clear how the check behaves and how to configure it, in a self-contained way!


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/operators-representation-to-traditional.cpp:6
+void testAllTokensToAlternative(int a, int b) {
+    int value = 0;
+
----------------
Nit: keep indentation to 2 spaces as is standard in the rest of the repo (including tests).


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/operators-representation-to-traditional.cpp:9
+    value = a or b;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'or' is an alternative token spelling, consider using a traditional token '||' for consistency [readability-operators-representation]
+// CHECK-FIXES: {{^    }}value = a || b;{{$}}
----------------
Nit: indent comments 2 spaces so they align with the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144522