<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 21, 2014 at 11:17 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=dblaikie@gmail.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Fri, Nov 21, 2014 at 10:47 AM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: rikka<br>
Date: Fri Nov 21 12:47:58 2014<br>
New Revision: 222549<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=222549&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=222549&view=rev</a><br>
Log:<br>
Use the full-Expr filter to disambiguate equidistant correction<br>
candidates.<br>
<br>
Modified:<br>
cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=222549&r1=222548&r2=222549&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=222549&r1=222548&r2=222549&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Fri Nov 21 12:47:58 2014<br>
@@ -5992,7 +5992,7 @@ class TransformTypos : public TreeTransf<br>
typedef TreeTransform<TransformTypos> BaseTransform;<br>
<br>
llvm::function_ref<ExprResult(Expr *)> ExprFilter;<br>
- llvm::SmallSetVector<TypoExpr *, 2> TypoExprs;<br>
+ llvm::SmallSetVector<TypoExpr *, 2> TypoExprs, AmbiguousTypoExprs;<br>
llvm::SmallDenseMap<TypoExpr *, ExprResult, 2> TransformCache;<br>
llvm::SmallDenseMap<OverloadExpr *, Expr *, 4> OverloadResolution;<br>
<br>
@@ -6059,6 +6059,15 @@ class TransformTypos : public TreeTransf<br>
return nullptr;<br>
}<br>
<br>
+ ExprResult TryTransform(Expr *E) {<br>
+ Sema::SFINAETrap Trap(SemaRef);<br>
+ ExprResult Res = TransformExpr(E);<br>
+ if (Trap.hasErrorOccurred() || Res.isInvalid())<br>
+ return ExprError();<br>
+<br>
+ return ExprFilter(Res.get());<br>
+ }<br>
+<br>
public:<br>
TransformTypos(Sema &SemaRef, llvm::function_ref<ExprResult(Expr *)> Filter)<br>
: BaseTransform(SemaRef), ExprFilter(Filter) {}<br>
@@ -6079,30 +6088,42 @@ public:<br>
ExprResult TransformLambdaExpr(LambdaExpr *E) { return Owned(E); }<br>
<br>
ExprResult Transform(Expr *E) {<br>
- ExprResult res;<br>
- bool error = false;<br>
+ ExprResult Res;<br>
while (true) {<br>
- Sema::SFINAETrap Trap(SemaRef);<br>
- res = TransformExpr(E);<br>
- error = Trap.hasErrorOccurred();<br>
-<br>
- if (!(error || res.isInvalid()))<br>
- res = ExprFilter(res.get());<br>
+ Res = TryTransform(E);<br>
<br>
// Exit if either the transform was valid or if there were no TypoExprs<br>
// to transform that still have any untried correction candidates..<br>
- if (!(error || res.isInvalid()) ||<br>
+ if (!Res.isInvalid() ||<br>
!CheckAndAdvanceTypoExprCorrectionStreams())<br>
break;<br>
}<br>
<br>
+ // Ensure none of the TypoExprs have multiple typo correction candidates<br>
+ // with the same edit length that pass all the checks and filters.<br>
+ // TODO: Properly handle various permutations of possible corrections when<br>
+ // there is more than one potentially ambiguous typo correction.<br>
+ while (!AmbiguousTypoExprs.empty()) {<br>
+ auto TE = AmbiguousTypoExprs.back();<br>
+ auto Cached = TransformCache[TE];<br>
+ AmbiguousTypoExprs.pop_back();<br>
+ TransformCache.erase(TE);<br>
+ if (!TryTransform(E).isInvalid()) {<br>
+ SemaRef.getTypoExprState(TE).Consumer->resetCorrectionStream();<br>
+ TransformCache.erase(TE);<br>
+ Res = ExprError();<br>
+ break;<br>
+ } else<br>
+ TransformCache[TE] = Cached;<br>
+ }<br>
+<br>
// Ensure that all of the TypoExprs within the current Expr have been found.<br>
- if (!res.isUsable())<br>
+ if (!Res.isUsable())<br>
FindTypoExprs(TypoExprs).TraverseStmt(E);<br>
<br>
EmitAllDiagnostics();<br>
<br>
- return res;<br>
+ return Res;<br>
}<br>
<br>
ExprResult TransformTypoExpr(TypoExpr *E) {<br>
@@ -6124,21 +6145,15 @@ public:<br>
State.RecoveryHandler(SemaRef, E, TC) :<br>
attemptRecovery(SemaRef, *State.Consumer, TC);<br>
if (!NE.isInvalid()) {<br>
- // Check whether there is a second viable correction with the same edit<br>
- // distance--in which case do not suggest anything since both are<br>
- // equally good candidates for correcting the typo.<br>
- Sema::SFINAETrap LocalTrap(SemaRef);<br>
+ // Check whether there may be a second viable correction with the same<br>
+ // edit distance; if so, remember this TypoExpr may have an ambiguous<br>
+ // correction so it can be more thoroughly vetted later.<br>
TypoCorrection Next;<br>
- while ((Next = State.Consumer->peekNextCorrection()) &&<br>
- Next.getEditDistance(false) == TC.getEditDistance(false)) {<br>
- ExprResult Res =<br>
- State.RecoveryHandler<br>
- ? State.RecoveryHandler(SemaRef, E, Next)<br>
- : attemptRecovery(SemaRef, *State.Consumer, Next);<br>
- if (!Res.isInvalid()) {<br>
- NE = ExprError();<br>
- State.Consumer->getNextCorrection();<br>
- }<br>
+ if ((Next = State.Consumer->peekNextCorrection()) &&<br>
+ Next.getEditDistance(false) == TC.getEditDistance(false)) {<br>
+ AmbiguousTypoExprs.insert(E);<br>
+ } else {<br>
+ AmbiguousTypoExprs.remove(E);<br>
}<br>
assert(!NE.isUnset() &&<br>
"Typo was transformed into a valid-but-null ExprResult");<br>
<br>
Modified: cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp?rev=222549&r1=222548&r2=222549&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp?rev=222549&r1=222548&r2=222549&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp Fri Nov 21 12:47:58 2014<br>
@@ -48,3 +48,14 @@ void testNoCandidates() {<br>
callee(xxxxxx, // expected-error-re {{use of undeclared identifier 'xxxxxx'{{$}}}}<br>
zzzzzz); // expected-error-re {{use of undeclared identifier 'zzzzzz'{{$}}}}<br>
}<br>
+<br>
+class string {};<br>
+struct Item {<br>
+ void Nest();<br>
+ string text();<br>
+ Item* next(); // expected-note {{'next' declared here}}<br>
+};<br>
+void testExprFilter(Item *i) {<br>
+ Item *j;<br>
+ j = i->Next(); // expected-error {{no member named 'Next' in 'Item'; did you mean 'next'?}}<br>
+}<br></blockquote></div></div><div><br>I imagine this test should pass by picking the best candidate (the edit distance from Next -> next is shorter than Next->text or Next-Nest), right? If that's the case, is that not addressed (are we not picking the best candidate?)? Once it is, we might need a different test case to verify this tie handling - presumably a test where the best candidate has a tie?<br><br>(but I'm guessing your example had something else to test - since only the return type of 'next' would make this compile - the other two return types don't match - what was that about? Clearly I'm missing something here)</div></div></div></div></blockquote><div><br></div><div>The edit distance from Next to all three of the candidates is the same--all are 1 char off--so this explicitly tests tie handling. Prior to this patch, no suggestion would ever be offered because, without considering the surrounding assignment expression, all three candidates are equally likely (all are a one character change, all are members of the same struct, all are methods that take zero arguments, all are accessible in the current context, etc) so no suggestion would be given. This patch allows the candidates to be narrowed down (in this case) based on the return type needed to make the assignment expression valid (since two of the three suggestions would create an invalid assignment, while the third works just fine). And the order of the methods is chosen so that the first candidate isn't being chosen by virtue of being first, as would happen with the delayed typo correction prior to r222462.</div></div><br></div></div>