<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jul 14, 2014 at 4:55 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">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"><br>
The the typo count is propagated up the stack of<br>
ExpressionEvaluationContextRecords when one is popped off of to<br>
avoid accidentally dropping TypoExprs on the floor. For example,<br>
the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp<br>
happens with an ExpressionEvaluationContextRecord that is popped off<br>
the stack prior to ActOnFinishFullExpr being called and the tree<br>
transform for TypoExprs being run.<br></blockquote><div><br></div><div>FWIW, that case sounds like a bug: finalizing the full-expression should happen within the relevant ExpressionEvaluationContext.</div><div><br></div>
<div>Is a simple count of TypoExprs sufficient here? If we leave the local expression without diagnosing a typo (or if our RecursiveASTVisitor fails to diagnose it), I think we still need to track it so that we can produce some kind of diagnostic for it. In particular:</div>
<div><br></div><div><div>+ assert(NumTypos == 0 && "There are outstanding typos after popping the "</div><div>+ "last ExpressionEvaluationContextRecord");</div></div>
<div><br></div><div>... I think there are probably cases where this assertion can fire, and the right thing to do seems to be to issue a basic 'unknown identifier' diagnostic for any remaining typos.</div><div><br>
</div><div><br></div><div>+ llvm::SmallVector<Expr *, 4> ExprStack;<br></div><div><br></div><div>This seems rather small; I expect we'd walk through more Exprs than this. But... you don't actually use this. Can you remove it?</div>
<div><br></div><div><br></div><div><div>+ // If corrections for the first TypoExpr have been exhausted, retry them</div><div>+ // against the next combination of substitutions for all of the other</div><div>+ // TypoExprs.</div>
<div>+ if (FirstTypoExpr->Consumer->finished()) {</div><div>+ unsigned numFinished = 1;</div><div>+ for (auto TE : TypoExprs) {</div><div>+ if (TE != FirstTypoExpr) {</div><div>+ TransformCache.erase(TE);</div>
<div>+ if (TE->Consumer->finished()) {</div><div>+ ++numFinished;</div><div>+ TE->Consumer->resetCorrectionStream();</div><div>+ } else {</div><div>+ break;</div>
<div>+ }</div><div>+ }</div><div>+ }</div><div>+ FirstTypoExpr->Consumer->resetCorrectionStream();</div><div>+ if (numFinished >= TypoExprs.size())</div><div>+ break;</div>
<div>+ }</div></div><div><br></div><div>It looks like the net result of this is that you'll try:</div><div><br></div><div> * Typo 1 correction 1, typo 2 correction 1, typo 3 correction 1</div><div> * Typo 1 correction 2, typo 2 correction 1, typo 3 correction 1</div>
<div> * ...</div><div> * Typo 1 correction 1, typo 2 correction 2, typo 3 correction 2</div><div> * Typo 1 correction 2, typo 2 correction 2, typo 3 correction 2<br></div><div> * ...</div><div><br></div><div>... and you'll never try typo 2 correction 2 with typo 3 correction 1. (Also, if typo 2 has 2 corrections and typo 3 has 3, you'll never consider the third correction for typo 3).</div>
<div><br></div><div>I think we need to decide:</div><div> * how many different combinations of typo corrections we're prepared to try, and</div><div> * which order to search them in</div><div><br></div><div>Due to the tree structure of the AST, we should be able to do better than trying all combinations; for instance, given:</div>
<div><br></div><div> f(typo1, g(typo2, typo3))</div><div><br></div><div>... we can figure out which combinations work for the call to g, and only try those combinations in the call to f.</div><div><br></div><div><br></div>
<div><div>@@ -5964,6 +6063,16 @@ ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC,</div><div> return ExprError();</div><div> }</div><div> </div><div>+ if (ExprEvalContexts.back().NumTypos &&</div>
<div>+ (FullExpr.get()->isTypeDependent() ||</div><div>+ FullExpr.get()->isValueDependent() ||</div><div>+ FullExpr.get()->isInstantiationDependent())) {</div><div>+ FullExpr = TransformTypos(*this).Transform(FullExpr.get());</div>
<div>+ ExprEvalContexts.back().NumTypos = 0;</div><div>+ if (FullExpr.isInvalid())</div><div>+ return ExprError();</div><div>+ }</div></div><div><br></div><div>This is assuming that any call to TransformTypos::Transform will remove all TypoExprs within the entire ExpressionEvaluationContext. I am not sure that this will necessarily be true in general. It'd be more robust for TransformTypos to specify which TypoExprs it transformed, and to remove those from the ExpressionEvaluationContext or from a list of currently undiagnosed TypoExprs.</div>
<div><br></div><div><br></div><div><div>+/// \returns a new \c TypoExpr that will later be replaced in the AST with an</div><div>+/// Expr representing the result of performing typo correction.</div><div>+TypoExpr *Sema::CorrectTypoDelayed(</div>
<div><br></div></div><div>It would be useful to document that the caller still needs to produce a diagnostic if this returns nullptr.</div></div></div></div>