Page MenuHomePhabricator

[Parser] only correct delayed typos for conditional expressions when needed.

Authored by dtarditi on Jul 28 2016, 12:53 PM.



r272587 ( fixed an issue where typo correction could cause a crash when compiling C programs. The problem was that a typo expression could be inadvertently processed twice. r272587 fixed this for BinOp expressions. Conditional expressions can hit the same problem too. This change adds the two line fix for conditional expressions, as well as a small test case illustrating the crash.

Note that I don't have commit privileges for clang; someone will need to commit this for me. I originally started the review by email and am shifting it over to Phrabicator. The change incorporates the following feedback from Erik Pilkington on the original patch submitted by email:

  1. If we’re doing the check at the end of both branches of the if, we might as well just move it to after.
  2. It doesn’t make sense to have a failure that only occurs in C mode in test/SemaCXX. Maybe just append it to the end of test/Sema/typo-correction.c.

    I see from the prior review that the consensus was that “typo correction is in a messy state, we should fix this”. I agree. There are other problematic places in the code where double-processing might or might not occur for C code. An example is the processing of subscript expressions in Parser::ParsePostfixExpressionSuffix. Without clear invariants, it is hard to know what to do.

Testing: clang fails on the test case without this change, passes with it. The clang test suite results were otherwise the same before and after this change.

Diff Detail


Event Timeline

dtarditi updated this revision to Diff 65965.Jul 28 2016, 12:53 PM
dtarditi retitled this revision from to [Parser] only correct delayed typos for conditional expressions when needed..
dtarditi updated this object.
dtarditi added a reviewer: erik.pilkington.
dtarditi added a subscriber: cfe-commits.
erik.pilkington accepted this revision.Jul 28 2016, 6:02 PM
erik.pilkington edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Jul 28 2016, 6:02 PM
This revision was automatically updated to reflect the committed changes.