Page MenuHomePhabricator

[clang-tidy] Fixed readability-else-after-return for cascade statements
ClosedPublic

Authored by idlecode on Oct 30 2016, 1:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

idlecode updated this revision to Diff 76328.Oct 30 2016, 1:30 AM
idlecode retitled this revision from to [clang-tidy] Fixed else-after-return warning in cascade if statement.
idlecode updated this object.
idlecode added a subscriber: cfe-commits.
omtcyfz edited reviewers, added: alexfh; removed: omtcyfz.Oct 30 2016, 2:29 AM
omtcyfz added a subscriber: omtcyfz.
djasper added inline comments.Oct 30 2016, 7:39 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
28 ↗(On Diff #76328)

I think this now effectively does:

stmt(forEach(ifStmt(unless(hasParent(ifSttmt())), ...)

I think that's equivalent to:

stmt(unless(ifStmt()), forEach(ifStmt(...)))
idlecode marked an inline comment as done.Oct 30 2016, 8:31 AM
idlecode added inline comments.
clang-tidy/readability/ElseAfterReturnCheck.cpp
28 ↗(On Diff #76328)

Apparently you are right - does that mean that this matcher actually matches node above ifStmt (and just binds ifStmt)?
If so maybe such expression would suffice? (it passes tests)

Finder->addMatcher( // note lack of stmt(forEach(
              ifStmt(unless(hasParent(ifStmt())),
                 hasThen(stmt(
                     anyOf(ControlFlowInterruptorMatcher,
                           compoundStmt(has(ControlFlowInterruptorMatcher))))),
                 hasElse(stmt().bind("else")))
              .bind("if"),
      this);

But I'm not sure if this would be any better than your version.

djasper added inline comments.Oct 30 2016, 8:53 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
28 ↗(On Diff #76328)

Yeah. That makes sense to me. The difference is that we would start binding ifStmts() that are direct children of declarations. But I don't know whether we have excluded those intentionally. Do any tests break if you make this change?

idlecode marked an inline comment as done.Oct 30 2016, 9:38 AM
idlecode added inline comments.
clang-tidy/readability/ElseAfterReturnCheck.cpp
28 ↗(On Diff #76328)

I have just ran make check-clang-tools on updated source tree - no new failures. Upload the change?
What do you mean by 'if statement as children of declaration'? (I'm new to compilers and just curious :) )

djasper added inline comments.Oct 30 2016, 9:49 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
28 ↗(On Diff #76328)

I did some more digging. It appears this bug was introduced in https://reviews.llvm.org/rL278257. The old code specifically did the forEach stuff to ensure that the ifStmt was a direct child of a compound stmt. I think we should investigate why this happened and probably undo it. There are similar to this one here if the ifStmt is a child of a for or while loop without a compound stmt.

idlecode planned changes to this revision.Oct 30 2016, 10:37 AM
idlecode added inline comments.
clang-tidy/readability/ElseAfterReturnCheck.cpp
28 ↗(On Diff #76328)

Indeed, my fix fails for loops.
Also, do you see any reason why 'goto' was not included in interrupt statements?

djasper added inline comments.Oct 30 2016, 10:45 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
28 ↗(On Diff #76328)

I don't think anyone cares about readability when there is "goto" ;)

mgehre added a subscriber: mgehre.Oct 30 2016, 12:26 PM

With this fix, is there still a warning on the following code?

if(b) {
   ...
   if(c)
    return;
   else
    doSomething()
   ...
}

I would expect that the check still warns on it.

@mgehre: Yes it does - in your case AST looks like ifStmt(b) - CompoundStmt - ifStmt(c), so ifStmt's don't have direct parent-child relations.
But there is another problem:

while(a)
  if (b)
    return 1;
  else
    doSomething(2);

In such case, generated fix (with and without this fix) will change semantics of program.
As @djasper suggested, I need to take a look at this check history and update patch accordingly.

idlecode updated this revision to Diff 76604.Nov 1 2016, 11:29 AM
idlecode retitled this revision from [clang-tidy] Fixed else-after-return warning in cascade if statement to [clang-tidy] Fixed readability-else-after-return for cascade statements.
idlecode updated this object.

I have reverted matcher to the state before https://reviews.llvm.org/D23265 (tests are passing with compoundStmt instead of stmt and this way is better than mine).

djasper accepted this revision.Nov 1 2016, 11:43 AM
djasper edited edge metadata.

Looks good. Thanks for fixing this!

This revision is now accepted and ready to land.Nov 1 2016, 11:43 AM
This revision was automatically updated to reflect the committed changes.