This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved
ClosedPublic

Authored by njames93 on Nov 14 2020, 2:10 PM.

Details

Summary

Consider this code:

if (Cond) {
#ifdef X_SUPPORTED
  X();
#else
  return;
#endif
} else {
  Y();
}
Z();

In this example, if X_SUPPORTED is not defined, currently we'll get a warning from the else-after-return check. However If we apply that fix, and then the code is recompiled with X_SUPPORTED defined, we have inadvertently changed the behaviour of the if statement due to the else being removed. Code flow when Cond is true will be:

X();
Y();
Z();

where as before the fix it was:

X();
Z();

This patch adds checks that guard against #endif directives appearing between the control flow interrupter and the else and not applying the fix if they are detected.

Diff Detail

Event Timeline

njames93 created this revision.Nov 14 2020, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2020, 2:10 PM
njames93 requested review of this revision.Nov 14 2020, 2:10 PM
njames93 updated this revision to Diff 305373.Nov 15 2020, 9:59 AM

Fix assertions failing with included files.
Ran this over the entire clang directory, which has a lot of macro (ab)use, and no crashes or otherwise unexpected behaviour.

aaron.ballman added inline comments.Nov 17 2020, 1:01 PM
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
118

Unrelated formatting change?

189

Should the map be passed by const ref to avoid copies?

203

Should this be const auto & to be clear there are no modifications to the map?

214

Missing a full stop at the end of the comment.

clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
268

Same -> same
Also, missing a full stop.

286

ensure -> Ensure
Also, missing a full stop.

305

diagnost -> diagnose

314

We should probably add some tests for more pathological cases, like:

#if 1
if (true) {
  return;
#else
{
#endif
} else {
  return;
}
njames93 marked 8 inline comments as done.Nov 17 2020, 1:32 PM
njames93 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
314

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 :

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.

njames93 updated this revision to Diff 305897.Nov 17 2020, 1:34 PM
njames93 marked an inline comment as done.

Address comments.

njames93 updated this revision to Diff 305898.Nov 17 2020, 1:35 PM

Whoops missed one.

aaron.ballman accepted this revision.Nov 18 2020, 7:13 AM

LGTM aside from the testing request.

clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
314

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.

This revision is now accepted and ready to land.Nov 18 2020, 7:13 AM
steveire requested changes to this revision.Nov 18 2020, 7:38 AM
steveire added a subscriber: steveire.
steveire added inline comments.
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
34

auto needs to be replaced/expanded.

208

auto needs to be replaced/expanded.

This revision now requires changes to proceed.Nov 18 2020, 7:38 AM
njames93 updated this revision to Diff 306143.Nov 18 2020, 10:16 AM

Added test cases to ensure clang-tidy doesn't crash.
Expanded auto out.

njames93 marked 3 inline comments as done.Nov 19 2020, 3:57 AM
aaron.ballman accepted this revision.Nov 19 2020, 5:30 AM

LGTM!

clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-pp-no-crash.cpp
2

You can drop the -- -std=c++11 from this.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 19 2020, 10:20 AM
This revision was automatically updated to reflect the committed changes.