Page MenuHomePhabricator

[Sema] SequenceChecker: Fix handling of operator ||, && and ?:

Authored by riccibruno on Feb 5 2019, 5:25 AM.



The current handling of the operators ||, && and ?: has a number of false positive and false negative. The issues for operator || and && are:

  1. We need to add sequencing regions for the LHS and RHS as is done for the comma operator. Not doing so causes false positives in expressions like ((a++, false) || (a++, false)); (from PR39779, see PR22197 for another example).
  1. In the current implementation when the evaluation of the LHS fails, the RHS is added to a worklist to be processed later. This results in false negatives in expressions like (a && a++) + a.

Fix these issues by introducing sequencing regions for the LHS and RHS, and by not deferring the visitation of the RHS.

The issues with the ternary operator ?: are similar, with the added twist that we should not warn on expressions like (x ? y += 1 : y += 2) since exactly one of the 2nd and 3rd expression is going to be evaluated, but we should still warn on expressions like (x ? y += 1 : y += 2) = y.

Diff Detail

Event Timeline

riccibruno created this revision.Feb 5 2019, 5:25 AM

Looking at git blame, this change means that we will warn on (b && x++) + (!b && x++). However we will also warn on an expression like (x > some_constant && y++) + (x < some_constant && y++). This seems to be the job of a static analyser which is able to do some control flow analysis. Moreover for this warning it seems to me that having some false negatives is worse than having some false positive. As a data point I ran this on the entire LLVM codebase and on all of Boost, and did not find any additional warning.

rsmith accepted this revision.Feb 5 2019, 9:03 AM

The "false negatives" in the current behaviour are the result of an intentional decision to avoid false positives for unsequenced operations that cannot actually both happen as part of the same evaluation (because both are conditional on different and mutually-exclusive conditions), such as the false positive we now get in the tests. I think it's reasonable to revisit that decision, though; the new false positive cases do not realistically seem likely to occur in real code whereas the false negatives do.

79 ↗(On Diff #185284)

Please at least add a FIXME for this false positive.

This revision is now accepted and ready to land.Feb 5 2019, 9:03 AM
riccibruno marked an inline comment as done.


Rebased on D57660. No need to look at it.

Does the whole stack of patch need to be commited at once or maybe you can land them individually?

Does the whole stack of patch need to be commited at once or maybe you can land them individually?

Hi, thanks for looking at this patch series !

If I remember correctly (it has been a while now) these patches could be landed independently. I was sort of hoping that D57660 would get accepted and that I could then land the other patches without rebasing them (the tests are especially annoying to rebase). That said if D57660 is a problem then I can totally do the work and land the other already accepted patches.

Rebased on top of current master and D57659. No need to look at it.

This revision was automatically updated to reflect the committed changes.