Add a check to flag loops and functions where an if at the end of the body could be negated and made into an early exit.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst | ||
---|---|---|
74 | Please add newline. |
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp | ||
---|---|---|
217 | I have an idea that we could infer this using the readability-braces-around-statements and its hicpp alias. |
Add option SplitConjunctions to alter fix-it for if statements with && in the condition.
Add NestedControlFlow options
This option can be used to silence diagnostics when there aren't any nested blocks inside the if statement.
just my 2 cents
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp | ||
---|---|---|
67 | did you consider comma expressions? if (myDirtyCode(), myCondition && yourCondition). It seems to me, that the transformation would be incorrect. | |
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst | ||
56 | typo cunjunctions -> conjunctions | |
64 | 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. | |
113 | maybe highlight the if as code with double quotes? |
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp | ||
---|---|---|
67 | Comma operator is a binary operator, so the transformation would wrap the whole expression in parens. | |
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst | ||
64 | 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. |
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst | ||
---|---|---|
64 | a short reference to the readability-simplify-boolean-expr check in the user facing docs would be great. 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 :) ) |
Fix documentation.
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst | ||
---|---|---|
64 | 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. |
_Edit_: If overloaded operators are ignored and the check only applies to pointers, int and bool the following is irrelevant. But I did not spot such a guard.
If this is the case, it of course opens the question about at least optional<> types and how to handle those.
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, both in this instance and in general for sure.
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.
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp | ||
---|---|---|
330 | i would really prefer to just use strings here. this function is called only once in the constructor as well, so speed and allocations are not valid in my opinion. | |
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst | ||
64 | ok, thats good with me. |
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.
if (!(A && B)) continue; if (!(!B && C)) continue; padLines();
The only potential issue would be cases when the binary operator is type dependent, as binary operators where they type is unresolved are handled as BinaryOperators, even if every instantiation would be resolved to an operator call
template <size_t N, size_t M> void fancyMatrix(Matrix<N,M> A, Matrix<M, M> B) { if (a && B) { ... } }
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?
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.
I'm not certain that the result of the transformation is more "readable"; is this check intended to aid conformance to a style guide?
One of the driving factors of this is that it can be used to remove nesting levels, which can definitely aid with readability.
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp | ||
---|---|---|
360 | This will trigger on all system code, and then users will complain again that they see poor clang-tidy performance... when it could be just something like ifStmt(unless(isExpansionInSystemHeader()), unless(isConsteval()), unless(isConstexpr()), unless(hasElse(stmt())), unless(hasInitStatement(stmt()), hasThen(compoundStmt(hasSizeAboeLineTreshold())), ... Simply everything that could be put into matcher should be put into matcher (easier to maintain), also what's a point of checking functions that doesn't have if's. On that point also some implicit functions or if statement should be ignored, to avoid checking templates twice. | |
clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp | ||
145 |
|
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp | ||
---|---|---|
360 | Not triggering on system headers is a good point, but for raw performance, that code is better residing in the visitor, which can massively outperform matchers as we can straight up ignore huge blocks of code that aren't of interest. |
did you consider comma expressions?
if (myDirtyCode(), myCondition && yourCondition). It seems to me, that the transformation would be incorrect.