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

Mon Sep 19 01:59:57 PDT 2022

JonasToth added a comment.

In D130181#3769618 <https://reviews.llvm.org/D130181#3769618>, @njames93 wrote:

> In D130181#3769083 <https://reviews.llvm.org/D130181#3769083>, @JonasToth wrote:
>
>> ...
>
> Your concerns aren't actually that important. Because the transformations only work on for binary operators, and not CXXOperatorCallExpr, it would always never do any special logic, instead just wrap the whole thing in parens and negate it.

Ah yes, you are right! That makes it very much better.

> I think the best way to resolve that could be delayed diagnostics for template operators.
> So cache the locations of all dependent binary (and unary) operators, and see if any of them either stay as binary operators or get transformed into CXXOperatorCallExprs, if so don't emit the fix.
> The second option is to just not emit any fix for template dependent operators.
> WDYT?

I am not 100% sure that delayed diagnostics would solve the issue _always_

  template <typename T1, typename T2>
  bool type_dependent_logic(T1 foo, T2 bar) {
      return foo && !bar;
  }

This template would be a counter example to yours, where I feel less comfortable.
The definition itself has `BinaryOperator` in the AST.

Now different TUs might use this template wildly different, e.g. TU1.cpp only with builtins, making a transformation possible and TU2.cpp with fancy and weird types making the transformation not possible.

clang-tidy (e.g. with `run-clang-tidy`) would now still change the template definition after it received the diagnostic from TU1.cpp, even though in TU2.cpp it is not considered as valid.
This issue is common to many checks, e.g. the `const-correctness` suffered from it as well. In the `const` case I had to just had to consider type dependent expressions as modifications.

For TU-local templates your strategy is certainly viable. But in the general case I would rather not do it, or at least have a "default-off" option for it.

Finally, what are your thoughts on `optional` types? They are kinda `bool` in the sense of this check, but suffer from the shortcomings with overloaded operators.
Should they be explicitly not considered now (i am fine with that!)? In that case, it should be mentioned as limitation in the documentation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130181