This is an archive of the discontinued LLVM Phabricator instance.

[SemObjC] Fix TypoExpr handling in TransformObjCDictionaryLiteral
ClosedPublic

Authored by bruno on Jul 8 2016, 5:58 PM.

Details

Summary

Calls to TransformExpr for NSDictionary elements (keys and values) in
TransformObjCDictionaryLiteral might fail to obtain TypoCorrections. This is OK,
but the early exits with ExprError() don't allow TransformExpr to run for other
elements.

This avoids typo correction suggestions to kick in and has a major
drawback to compile time; it forces Transform to call TryTransform more
times than it should. Before this patch, the testcase added used to take
5s to compile!!! A bit more elaborate NSDictionary literal with some
undeclared enums would make the compiler take 22min to run, followed by
a crash.

Diff Detail

Event Timeline

bruno updated this revision to Diff 63363.Jul 8 2016, 5:58 PM
bruno retitled this revision from to [SemObjC] Fix TypoExpr handling in TransformObjCDictionaryLiteral.
bruno updated this object.
bruno added reviewers: manmanren, doug.gregor.
bruno added a subscriber: cfe-commits.
manmanren edited edge metadata.Jul 12 2016, 11:20 AM

Before this patch, the testcase added used to take
5s to compile!!! A bit more elaborate NSDictionary literal with some
undeclared enums would make the compiler take 22min to run, followed by a crash.

--> this is a big improvement!

A few notes from discussions with Bruno:
This patch changes the base class TreeTransform<Derived>::TransformObjCDictionaryLiteral, which is not specific to TransformTypos.

Here we have multiple TypoExprs in a single DictionaryLiteral, and the search space for each TypoExpr can be pretty big. That is why it takes a long time to go through all the possible corrections.

I applied your patch, looks like it returns an ObjCDictionaryLiteral that contains TypoExpr as valid and that is why we break out of the while loop in TransformTypos::Transform. That does not look right.

We can probably prune the search space if one of the TypoExprs has zero candidate (in the testing case, the 2nd TypoExpr has zero candidate).

Manman

bruno updated this revision to Diff 64162.Jul 15 2016, 10:40 AM
bruno edited edge metadata.

Thanks for the review Manman, I found a much better approach. Updated the patch to reflect that!

manmanren accepted this revision.Jul 18 2016, 5:12 PM
manmanren edited edge metadata.

LGTM.

Manman

lib/Parse/ParseObjc.cpp
3489 ↗(On Diff #64162)

Add a period at the end of the comment.

This revision is now accepted and ready to land.Jul 18 2016, 5:12 PM
bruno closed this revision.Jul 19 2016, 1:29 PM

Thanks Manman, Committed r276020