[PATCH] D91485: [clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved

[Aaron Ballman via Phabricator via cfe-commits]

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