Page MenuHomePhabricator

[clang-tidy] Readability check for redundant parenthesis in return expression.
Needs RevisionPublic

Authored by adek05 on Jan 17 2016, 10:14 PM.

Details

Summary

Follows rough guidelines from PR22416. So far, I use very simple measure for expressions's simplicity. If it doesn't have > 1 children it is a simple one. Probably it also makes sense to check how deep is the tree if there is just one immediate child. This would catch situations like return (!(*foo));

Depends on D16278 which adds ParenExpr matcher.

Diff Detail

Event Timeline

adek05 updated this revision to Diff 45150.Jan 17 2016, 10:14 PM
adek05 retitled this revision from to [clang-tidy] Readability check for redundant parenthesis in return expression..
adek05 updated this object.
adek05 updated this revision to Diff 45151.Jan 17 2016, 10:19 PM

drop -check in the registered name for the checker

omtcyf0 added inline comments.
clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
51

Both warnings correspond to the same issue. Therefore I'd suggest creating one warning only.

LegalizeAdulthood added inline comments.
clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
32

Current AST matcher reference doesn't list parenExpr. That's what I use to find all the matchers. Why isn't this one documented?

42–45

Rather than using an ad-hoc heuristic, I'd rather the check just do it.

If we must have an ad-hoc heuristic, then we should have a configuration parameter that configures the metric ChildrenCountThreshold, where some value like 0 means "always do it" and this value should be the default value for this parameter.

If your return expression is so huge that you think you need extra parentheses to clarify something, then the problem isn't made any worse by removing the outer-most set of parentheses. The real problem of readability is your huge expression. Extracting subexpressions into a variable with an intention-revealing name is a better approach to readability. This is beyond the scope of this check.

test/clang-tidy/readability-return-with-redundant-parens.cpp
25

I don't know if it makes a difference, but I added a test case for:

return(1);

as well. In that case we need to remove the parens but add a space between the return and it's expression.

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
32

Oh, I missed in your description that this depends on another changeset that adds the matcher. Great!

adek05 added inline comments.Jan 18 2016, 9:30 AM
clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
32

I added it: D16278. Waiting to have it committed.

42–45

So you prefer moving this logic into check function? I separated that for the sake of readability. I do not mean that this is what we need. But for example we could expand the isComplexExpression to allow for return (!*foo); to be in parens (deep expression tree). I agree, that configuration param makes sense in such case. Which way would you prefer it?

51

I'll try to merge them.

test/clang-tidy/readability-return-with-redundant-parens.cpp
25

Yeah... that's an odd case. Ideally I would defer to clang-format here which could first add space between return and (1), but since it is easy enough I could just always do s/)/ / since clang-format is expected to be run after clang-tidy anyway. Good catch!

Eugene.Zelenko added inline comments.Jan 18 2016, 1:17 PM
test/clang-tidy/readability-return-with-redundant-parens.cpp
25

I encountered such statements in my work code base. Unfortunately I could not use Clang-format, since we standardized on Artistic Style, and its default didn't fix excessive spacing :-(

Eugene.Zelenko set the repository for this revision to rL LLVM.
clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
42–45

I don't see return (!*foo); as any clearer than return !*foo;. It's the !* that is the source of the complexity and adding parens just makes it look even more complicated, so I would rather that clang-tidy just remove the parentheses always.

In this case, I would "do the simplest thing that could possibly work" and always remove the parentheses.

Specifically, in this changeset I'm proposing that the function isComplexExpression be removed and the if statement on lines 41-44 also be removed and that the check simply always removes the parentheses.

51

What I would do is replace this with a single diagnostic:

diag(LParenLoc, "redundant parentheses for expression in return statement")
      << FixItHint::CreateReplacement(LParenLoc, "")
      << FixitHint::CreateReplacement(RParenLoc, "");

Also, I think the FixitHint stuff has a way of expressing a deletion directly instead of replace some text with empty string.

test/clang-tidy/readability-return-with-redundant-parens.cpp
25

In general, I wouldn't want to rely on clang-format to pretty up the input to clang-tidy for exactly the reasons that there are many code bases where it is unacceptable to run clang-format on the code base. At least not until clang-format subsumes all the functionality of other existing formatters (and it is far from that currently).

IMO, clang-tidy should be agnostic as to input formatting for what it can handle.

I think this check is a bit too aggressive about suggesting removal of parens. For instance:

return (foo && (bar || baz));

It seems reasonable that with more complex subexpressions involving operators of differing precedence that a user may want to keep the outer parens for clarity and that removing them actually reduces readability. Perhaps an option is in order (uncertain what to default its value to) to support that?

Also, how well does this interact with macro expansions, where parens are like candy?

#define FOO(x) (x)

void f() {
  return FOO(12)
}
clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
26

No need for ast_matchers.

test/clang-tidy/readability-return-with-redundant-parens.cpp
24

IMO, clang-tidy should be agnostic as to input formatting for what it can handle.

Strongly agreed.

alexfh edited edge metadata.Jan 19 2016, 7:14 AM

A high-level comment: why do we want to limit this check to only remove parentheses from return ()? Anything wrong with removing unnecessary parentheses everywhere (except for macro bodies)?

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
28

Please use const auto *: repeating the type name doesn't make anything clearer.

31

SourceLocations are small and trivially copyable, so they should be passed by value.

test/clang-tidy/readability-return-with-redundant-parens.cpp
1

Please add tests with macros:

  1. where superfluous parentheses are in the macro body (and they usually shouldn't be removed);
  2. where superfluous parentheses are in a macro argument (which is not itself located in a macro body (e.g. EXPECT_EQ(x, (y + z));) - I'd say that in this case we usually can remove parentheses;

Please add a test to ensure replacements are only applied once in templates with multiple instantiations.

With readability checks, there are always people who disagree about what makes for more readable code. I think I have seen this back-and-forth discussion with every readability check since I've been paying attention. It simply isn't possible to make everyone happy and we shouldn't even try.

As I stated earlier on this review, IMO the way to deal with complex expressions is not to shotgun blast extra parentheses into the expression, but to extract variables or functions with intention revealing names that break out meaningful subexpressions.

For checks relating to readability or style, I think it is fair to simply assume that the results are subjective. At the risk of sounding tautological, those who don't want the transformation shouldn't ask clang-tidy to perform the transformation.

There are already tons of transformations that clang-tidy performs that aren't to my personal taste, but are there because of a particular style guide (LLVM, google, etc.) or because the author of the check desires the transformation that is implemented.

I don't think such transformations should be denied from clang-tidy simply because of a difference of opinion.

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

@alexfh: The check and the bug report originate in the preferences of the google style guide.

With readability checks, there are always people who disagree about what makes for more readable code. I think I have seen this back-and-forth discussion with every readability check since I've been paying attention. It simply isn't possible to make everyone happy and we shouldn't even try.

It is perfectly reasonable for people to disagree about what makes code more or less readable. Some readability issues are more obvious than others, and the threshold for "readable" is going to change from check to check. However, that is not a reason to not attempt to make a check that makes everyone reasonably (un)happy.

As I stated earlier on this review, IMO the way to deal with complex expressions is not to shotgun blast extra parentheses into the expression, but to extract variables or functions with intention revealing names that break out meaningful subexpressions.

That seems reasonable to me, but this checker is *removing* parens, not adding them. The point I was making before is that when a subexpression is sufficiently complex, parens are a quick-and-dirty way to reduce cognitive load for understanding operator precedence. This is an approach you see relatively frequently -- a quick look at the LLVM source base shows that. For a check that claims it will make something more readable, removing code the user wrote had better have a pretty good chance of improving readability or else the check will quickly get disabled.

A good way to empirically test this is to run this check over the LLVM source base (and/or a few other large source bases) and see how often the diagnostic is triggered, and take a random sampling to see how often the removal of parens involves an expression with multiple operators of differing precedence to see if perhaps an option or design change is warranted or not. These sort of metrics are something we usually look for with any clang-tidy check that may have a high false-positive rate. For stylistic checks, I suppose just about anything could qualify as a false-positive (unless it's part of a style guideline that's being followed).

For checks relating to readability or style, I think it is fair to simply assume that the results are subjective.

Definitely agreed.

At the risk of sounding tautological, those who don't want the transformation shouldn't ask clang-tidy to perform the transformation.

That presumes the user knows what transformations this check will apply. If this was misc-remove-redundant-parens or some-style-guide-remove-redundant-parens, I would agree with your statement. However, with readability-* I think the user is likely to ask clang-tidy to perform the check and apply changes on a case-by-case basis. So we may want to start with the smallest set of cases where the check might increase readability and then expand from there to cover more cases as uses are requested.

There are already tons of transformations that clang-tidy performs that aren't to my personal taste, but are there because of a particular style guide (LLVM, google, etc.) or because the author of the check desires the transformation that is implemented.

I don't think such transformations should be denied from clang-tidy simply because of a difference of opinion.

No one is recommending denying this transformation; just discussing the particulars of it to ensure we have reasonable semantics for the check.

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

If it is meant to work for a particular style guide, then it should absolutely go under that style guide. But for something more generally under the readability-* umbrella, I think it doesn't hurt to discuss where to draw the line, or what options may be valuable.

As a general comment, I am continually fascinated by the huge variety of opinion as to matters of readability within the C++ community. I don't know why we have so many differing opinions compared to the communities built around other programming languages, but I enjoy hearing all of them. I enjoy having my opinions on readability challenged because it forces me to think why I find one form preferable over another and that is a good thing.

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

If it is meant to work for a particular style guide, then it should absolutely go under that style guide. But for something more generally under the readability-* umbrella, I think it doesn't hurt to discuss where to draw the line, or what options may be valuable.

I think that is the correct conclusion here; according to the bug report this check is motivated by the Google style guide which explicitly prohibits the parens for return expressions.

As a general comment, I am continually fascinated by the huge variety of opinion as to matters of readability within the C++ community. I don't know why we have so many differing opinions compared to the communities built around other programming languages, but I enjoy hearing all of them. I enjoy having my opinions on readability challenged because it forces me to think why I find one form preferable over another and that is a good thing.

Likewise! I think these discussions have been great (and I apologize if they are a source of frustration while we wrestle with the details).

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

If it is meant to work for a particular style guide, then it should absolutely go under that style guide. But for something more generally under the readability-* umbrella, I think it doesn't hurt to discuss where to draw the line, or what options may be valuable.

I think that is the correct conclusion here; according to the bug report this check is motivated by the Google style guide which explicitly prohibits the parens for return expressions.

Then perhaps this should go under google-* instead of readability-*? However, I posed an idea in a different review about a more general solution to the problem that may make sense under readability-* with specific options for an alias under google-*. Perhaps it would make sense to make this a general "strip *all* parens that don't have an effect on the order of evaluation in all expressions" check, which leaves room for an additional (different) check in the future that says "put parens in anywhere based on this heuristic we can debate in the future". I would say that these two checks should be off-by-default since they will definitely conflict, but I think the alias under google-* should be on by default and only focus on return statements (or whatever else the google style guide requires).

(Not saying that you have to implement this -- I'm just exploring the design space. The initial implementation of this idea could simply be to take your existing check and put it under google-* and we can refactor some day in the future for a more generalized readability-* check.)

jbcoe added a subscriber: jbcoe.Feb 12 2016, 1:57 PM
jbcoe removed a subscriber: jbcoe.
alexfh requested changes to this revision.Mar 24 2016, 7:45 AM
alexfh edited reviewers, added: aaron.ballman; removed: djasper, klimek.
alexfh edited edge metadata.
This revision now requires changes to proceed.Mar 24 2016, 7:45 AM