aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from the testing request.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:313
+#endif
+}
----------------
njames93 wrote:
> aaron.ballman wrote:
> > We should probably add some tests for more pathological cases, like:
> > ```
> > #if 1
> > if (true) {
> > return;
> > #else
> > {
> > #endif
> > } else {
> > return;
> > }
> > ```
> I'm don't even think its worth adding tests for that kind of code. Its
> impossible to reason about at the AST level(we dont get any AST for discarded
> pp conditional branches). So for these or cases like :
> ```lang=c++
> if (true) {
> #if 1
> return;
> } else {
> #endif
> return;
> }
> ```
> Its just simpler to leave the behaviour as is and hope that a user sees the
> change and addresses (or suppresses) it manually.
I was not suggesting that we add the tests because we want to elegantly handle
such eldritch horrors, but because I want to be sure we're not going to
crash/assert/etc if we encounter them. My suggestion is to add the test cases
and if you think the behavior isn't ideal, just stick a FIXME comment on the
test case to show that we don't like the behavior but aren't dealing with it
right now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91485/new/
https://reviews.llvm.org/D91485
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
