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: > > > > 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. https://reviews.llvm.org/D26125 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits