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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
