LegalizeAdulthood added inline comments.
================
Comment at: clang-tidy/readability/ReturnWithRedundantParensCheck.cpp:41-44
@@ +40,6 @@
+
+ // Complex expression can be surrounded by parenthesis for readability
+ if (isComplexExpression(ParenExpression->getSubExpr())) {
+ return;
+ }
+
----------------
adek05 wrote:
> LegalizeAdulthood wrote:
> > 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.
> 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?
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.
================
Comment at: clang-tidy/readability/ReturnWithRedundantParensCheck.cpp:50
@@ +49,3 @@
+ << FixItHint::CreateReplacement(LParenLoc, "");
+ diag(RParenLoc, "redundant ')' for expression in return statement")
+ << FixItHint::CreateReplacement(RParenLoc, "");
----------------
adek05 wrote:
> omtcyf0 wrote:
> > Both warnings correspond to the same issue. Therefore I'd suggest creating
> > one warning only.
> I'll try to merge them.
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.
================
Comment at: test/clang-tidy/readability-return-with-redundant-parens.cpp:24
@@ +23,2 @@
+ return (1 + 2);
+}
----------------
Eugene.Zelenko wrote:
> adek05 wrote:
> > LegalizeAdulthood wrote:
> > > 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.
> > 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!
> 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 :-(
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.
Repository:
rL LLVM
http://reviews.llvm.org/D16286
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
