djasper added inline comments.
================
Comment at: clang-tidy/readability/ElseAfterReturnCheck.cpp:28
stmt(forEach(
- ifStmt(hasThen(stmt(
+ ifStmt(unless(hasParent(ifStmt())),
+ hasThen(stmt(
----------------
idlecode wrote:
> djasper wrote:
> > idlecode wrote:
> > > djasper wrote:
> > > > idlecode wrote:
> > > > > djasper wrote:
> > > > > > I think this now effectively does:
> > > > > >
> > > > > > stmt(forEach(ifStmt(unless(hasParent(ifSttmt())), ...)
> > > > > >
> > > > > > I think that's equivalent to:
> > > > > >
> > > > > > stmt(unless(ifStmt()), forEach(ifStmt(...)))
> > > > > 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.
> > > > 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?
> > > 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 :) )
> > 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.
> Indeed, my fix fails for loops.
> Also, do you see any reason why 'goto' was not included in interrupt
> statements?
I don't think anyone cares about readability when there is "goto" ;)
https://reviews.llvm.org/D26125
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
