[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

[Jonas Toth via Phabricator via cfe-commits]

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