carlosgalvezp added inline comments.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability/operators-representation.rst:16
+To configure the check to enforce the traditional token representation, you can
+set the `BinaryOperators` and `OverloadedOperators` options to ``&&,||,!``,
+respectively. This will cause the check to warn on any occurrences of ``and``,
----------------
PiotrZSL wrote:
> carlosgalvezp wrote:
> > Is there any reason why this distinction is needed? The usage of the
> > operator happens in client code - it's unlikely one would want different
> > style depending on whether the operator is overloaded or not?
> >
> > ```
> > bool x = some_bool and some_other_bool;
> > bool y = some_object && some_other_object;
> > ```
> Consider code like this:
>
> ```
> template<typename T>
> void archive(T&& a, std::optional<int>& v)
> {
> a & v;
> }
>
> and
>
> bool test(unsigned value, unsigned mask)
> {
> return value & mask;
> }
> ```
>
> Those are 2 different use-cases, probably you don't want to use bit_and, but
> you could use bit_and for value & mask.
Good point, thanks for the clarification!
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability/operators-representation.rst:28-29
+
+Alternative Token Representation
+--------------------------------
+
----------------
carlosgalvezp wrote:
> PiotrZSL wrote:
> > carlosgalvezp wrote:
> > > Personally I find this to be a matter of style, and arguments could be
> > > found for either style. As such I don't think this check should promote
> > > the style preference of the author of the check.
> > >
> > > It's like having a written discussion in the clang-format option
> > > `ColumnLimit` explaining why N characters is a better choice than M
> > > characters. In the end this is a decision each project should take, and
> > > the check should not have a bias towards a preferred choice.
> > Following this way of thinking none of value is correct. Always will be
> > group that is for it or against it. Initially this check had to be only to
> > enforce alternative tokens but then made it configurable and change name of
> > check. This check is for "readability" and alternative tokens are more
> > readable. If someone don't like them, their choose, they can change config
> > to their needs.
> FYI @njames93 , do you have an opinion on the matter as Code Owner?
> alternative tokens are more readable
I don't believe this is an objective, universal truth - I think it's very
subjective, and you will find people who disagrees.
Take for example the "readability-identifier-naming" check. There's many
different conventions supported there, there's no "correct" one and "incorrect"
one. Imagine if the author wrote a 100+ lines dissertation about why Hungarian
notation is better than the others, and encourage people to choose that.
> if someone don't like them, their choose, they can change config to their
> needs.
Yes, this is exactly what I would expect from this check. My point is that this
lengthy discussion here encouraging choosing one style over the other is out of
place, cluttering the documentation with content that is not relevant to
understand what the check does and how to use it. The check documentation
should be neutral and just present the available options. This is consistent
with all other checks.
The purpose of this check is not to transform code into ATR style, but rather
to ensure consistency in a project, whichever style is wanted.
================
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`.
+
----------------
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.
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
