Page MenuHomePhabricator

-Wunreachable-code: Avoid multiple diagnostics that are triggered by the same source range and fix the unary operator fixit source range
ClosedPublic

Authored by arphaman on Jan 3 2017, 6:20 AM.

Details

Summary

This patch fixes an issue with -Wunreachable-code diagnostic that happens with the following code sample:

struct s {
  void *p;
};

void fn(struct s *s) {
  if (1 || !s->p)
    return;
  fn(s);
}

Currently, clang will emit two warnings (at fn(s) and at !s->p). As well as that, the fixits for the first warning (for fn(s)) have an incorrect source range: it suggests to wrap !s->p in parentheses when it should suggest to wrap 1 instead.

The attached patch ensures that the fixits for the fn(s) warning suggest to wrap the correct expression (1). It also avoids the second warning that was previously shown for !s->p because that warning is caused by the same expression (1).

Thanks for taking a look.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 82873.Jan 3 2017, 6:20 AM
arphaman retitled this revision from to -Wunreachable-code: Avoid multiple diagnostics that are triggered by the same source range and fix the unary operator fixit source range.
arphaman updated this object.
arphaman added reviewers: rsmith, bruno, ahatanak.
arphaman set the repository for this revision to rL LLVM.
arphaman updated this object.
arphaman added a subscriber: cfe-commits.
ahatanak edited edge metadata.Jan 3 2017, 12:28 PM

If I change the condition to the following,

if (!s->p || 1)

clang suggests enclosing !s->p with a parenthesis, but the comment in ReachableCode.cpp says the parenthesis should enclose the integer literal. It seems like there is a contradiction here?

arphaman updated this revision to Diff 83032.Jan 4 2017, 4:36 AM
arphaman edited edge metadata.

You're right, the fixit for if (!s->p || 1) is wrong, but that was another existing bug. The updated patch fixes this issue.

ahatanak added inline comments.Jan 5 2017, 2:41 PM
lib/Analysis/ReachableCode.cpp
229 ↗(On Diff #83032)

Thanks, that fixed the incorrect fixit. The patch looks good to me, but there are still cases in which the suggestions clang makes are not accurate. Perhaps you can leave a FIXME somewhere so that we don't forget to fix it later?

For example, if we change the condition in the test case to this,

if (!(s->p && 0))

, clang suggests enclosing the entire expression with a parenthesis (caret points to "!"), but the warning will not go away unless the parenthesis is around "0".

The second example is in function unaryOpFixit in warn-unreachable.c. clang suggests enclosing "-1" with a parenthesis, but it still warns after putting the parenthesis around "-1" or "1".

arphaman updated this revision to Diff 83819.Jan 10 2017, 9:34 AM

Fix more issues with FIXIT for unary operators.

arphaman added inline comments.Jan 10 2017, 9:57 AM
lib/Analysis/ReachableCode.cpp
229 ↗(On Diff #83032)

Thanks for pointing out these issues, I've decided to fix them in this patch. Since only the '!' operator can wrap a silenced expression, the updated patch ensures that fixits for unary operators can only be used with '!' (this fixes your second example). I fixed the first example by ensuring that the unary operator source range can be used for fixit only if the previous fixit source range comes from its direct child.

ahatanak accepted this revision.Jan 11 2017, 11:40 PM
ahatanak edited edge metadata.

Thanks, LGTM

This revision is now accepted and ready to land.Jan 11 2017, 11:40 PM
This revision was automatically updated to reflect the committed changes.