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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL planned changes to this revision.
PiotrZSL added a comment.

Update documentation, and config examples, add "Traditional Tokens Representation" section.



================
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`.
+
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144522