readability-simplify-boolean-expr: provide number of returns if expression found in return

[EugeneZelenko]

Bugzilla Link	24930
Version	unspecified
OS	All
CC	@LegalizeAdulthood

Extended Description

I think will be good idea to output number of return statements in function if readability-simplify-boolean-expr detect expression in return statements.

There are a lot of places in LLVM/Clang code with multiple returns, so fixing readability-simplify-boolean-expr may be unreasonable, since new exit points could be added in future.

[LegalizeAdulthood]

This doesn't match the "chained return" case because there are intermediate statements between the if statements.

[EugeneZelenko]

from include/llvm/Support/YAMLTraits.h

inline bool isNumber(StringRef S) { static const char OctalChars[] = "01234567"; if (S.startswith("0") && S.drop_front().find_first_not_of(OctalChars) == StringRef::npos) return true;

if (S.startswith("0o") && S.drop_front(2).find_first_not_of(OctalChars) == StringRef::npos) return true;

static const char HexChars[] = "0123456789abcdefABCDEF"; if (S.startswith("0x") && S.drop_front(2).find_first_not_of(HexChars) == StringRef::npos) return true;

static const char DecChars[] = "0123456789"; if (S.find_first_not_of(DecChars) == StringRef::npos) return true;

if (S.equals(".inf") || S.equals(".Inf") || S.equals(".INF")) return true;

Regex FloatMatcher("^(\.[0-9]+|[0-9]+(\.[0-9]*)?)([eE][-+]?[0-9]+)?$"); if (FloatMatcher.match(S)) return true;

return false; }

I don't wont to fix such code, just ignore based on number of returns.

[LegalizeAdulthood]

Please attach the specific original code and suggested fixit for reference as the LLVM code base is constantly shifting and just saying "this file" isn't a fixed reference.

[EugeneZelenko]

Are you sure you are running the latest version of the code?

There was feedback during review about "chained return statements" and "chains conditional assignments" and a configuration parameter was added to disable/enable changing such returns and assignments with the default being disabled.

I was not aware of this parameter. I'll try it when I'll have opportunity.

I didn't change defaults for chaining options, so chaining should be ignored in my runs.

See MIParser::parseBlockAddressOperand() in lib/CodeGen/MIRParser/MIRParser.cpp (LLVM trunk) for case which gave me idea of this improvement request.

[EugeneZelenko]

Are you sure you are running the latest version of the code?

There was feedback during review about "chained return statements" and "chains conditional assignments" and a configuration parameter was added to disable/enable changing such returns and assignments with the default being disabled.

I was not aware of this parameter. I'll try it when I'll have opportunity.

[LegalizeAdulthood]

Are you sure you are running the latest version of the code?

There was feedback during review about "chained return statements" and "chains conditional assignments" and a configuration parameter was added to disable/enable changing such returns and assignments with the default being disabled.

[EugeneZelenko]

I don't understand what is being requested here. Can you provide a code example to make it more concrete?

When I run this check over LLVM/Clang code I encountered several functions with a lot of return statements with false/true. SO there is no point to fix last return.

[LegalizeAdulthood]

I don't understand what is being requested here. Can you provide a code example to make it more concrete?

[LegalizeAdulthood]

@EugeneZelenko 5 years ago (:)) the discussion ended with me asserting that the code you supplied as an example of the problem doesn't apply because intermediate statements prevent the application of the check.

Is this still an issue?

[EugeneZelenko]

I didn't run Clang-tidy over LLVM code for awhile, so new experiment is needed.

[EugeneZelenko]

Yes, issue is still there. You could try this check on llvm/lib/Transforms/Scalar/LoopUnswitch.cpp in main.

[LegalizeAdulthood]

Thanks!

[LegalizeAdulthood]

OK, I looked through that entire file in my current tree and there aren't any situations where the check could apply. All the functions that return true or false explicitly have intermediate statements between the paired return statements. Therefore, the check can't apply because it would be deleting statements that shouldn't be deleted.

Is there a particular function in this file that you think should be changed by this check?

[LegalizeAdulthood]

In my tree MIParser::parseBlockAddressOperand() does have a situation where the check could be applied at the end.

Going back to the original request, I think this should be a separate readability check where it warns if the number of returns in a single scope is above a certain threshold. If you insist on a "single point of return" as a matter of style then you set the threshold to one.

[LegalizeAdulthood]

@EugeneZelenko Can you update the issue summary to reflect a new check instead of an option on readability-simplify-boolean-expr?

What do you think of readability-return-path-count as a name for the check?

[EugeneZelenko]

I still think that it's good idea to have an option in existing check, so in some configuration only simple situations will be considered (two returns) and more complicated logic ignored, in other - all of them.