JonasToth added a comment.
IMHO the check looks good, but I do have concerns about correctness of the
transformation in specific cases, especially overloaded operators.
If the conditions are inverted, it might be the case that the inverted operator
is not overloaded, resulting in compilation errors.
I think that should be guarded against.
And then there is the more subtle issue of different semantics of the operators.
Hypothetical Matrix Algebra Situation:
// A, B are matrices of booleans with identical dimensions
// A && B will do '&&' on each matrix element
// The matrices overload 'operator bool()' for implicit conversion to 'bool',
// true := any element != false
// false := all false
if (A && B) {
// !C inverts each boolean in C, same '&&' application
if (B && !C) {
padLines();
}
}
Transformed to
if (!A )
continue;
if ( !B)
continue;
if (!B )
continue;
if ( C)
continue;
padLines();
is highly likely not a correct and equivalent transformation.
My point is not so much about the concrete example, but more general.
C++ allows to implement operators with different semantics and classes must not
behave like 'int' for the operators. Such overloads are not necessarily
incorrect. E.g. `boost::sml` uses the operator overloading to define state
machines, which does not follow anything close to the semantics we need for
this check.
Expression-template libraries (especially linear algebra and so on) might
either break or change meaning as well.
In my personal opinion the check should at least have an option to disable
transformations for classes with overloaded operators and verify that the
transformation would still compile if done anyway.
I think even better would be a "concept check" to at least verify that the type
in question models a boolean with its operators. But I am not sure how far such
a check should go and I am aware that it would not be perfect anyway.
================
Comment at: clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp:329
+ assert(BSize >= CheckAlias.size() + OptName.size());
+ memcpy(Buff, CheckAlias.data(), CheckAlias.size());
+ memcpy(Buff + CheckAlias.size(), OptName.data(), OptName.size());
----------------
i would really prefer to just use strings here.
`std::string Buff = CheckAlias.str() + OptName.str();` is much easier to
understand and equivalent (?)
this function is called only once in the constructor as well, so speed and
allocations are not valid in my opinion.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst:63
+ void Process(bool A, bool B) {
+ if (A && B) {
+ // Long processing.
----------------
njames93 wrote:
> JonasToth wrote:
> > njames93 wrote:
> > > JonasToth wrote:
> > > > if this option is false, the transformation would be `if(!(A && B))`,
> > > > right?
> > > >
> > > > should demorgan rules be applied or at least be mentioned here? I think
> > > > transforming to `if (!A || !B)` is at least a viable option for enough
> > > > users.
> > > Once this is in, I plan to merge some common code with the
> > > simplify-boolean-expr logic for things like demorgan processing. Right
> > > now the transformation happens, the simplify boolean suggests a demorgan
> > > transformation of you run the output through clang tidy.
> > a short reference to the `readability-simplify-boolean-expr` check in the
> > user facing docs would be great.
> > i personally find it ok if the users "pipe" multiple checks together to
> > reach a final transformation.
> >
> > would this check then use the same settings as the
> > `readability-simplify-boolean-expr` check? (that of course off topic and
> > does not relate to this patch :) )
> I'm not sure it's really necessary to mention that the fix would likely need
> another fix, besides that comment would just be removed in the follow up.
ok, thats good with me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130181/new/
https://reviews.llvm.org/D130181
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
