[PATCH] D16286: [clang-tidy] Readability check for redundant parenthesis in return expression.

[Richard via cfe-commits]

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