<div dir="ltr"><div>+    /// \brief The number of typos encountered during this expression evaluation<br></div><div>+    //context (i.e. the number of TypoExprs created).</div><div><br></div><div>// should be ///.</div><div>
<br></div><div><br></div><div><div>+  bool trapping = false;</div></div><div><br></div><div>Capital T. Also, MSVC doesn't accept this; please put the initializer in the constructor.</div><div class="gmail_extra"><br></div>
<div class="gmail_extra"><div class="gmail_extra">+  ExprResult TransformLambdaExpr(LambdaExpr *E) { return Owned(E); }</div><div><br></div><div>Do you need to do this for blocks and CapturedStmts too?</div><div><br></div>
<div><div>+    while (!E->Consumer->empty()) {</div><div>+      if (TypoCorrection TC = E->Consumer->getNextCorrection()) {</div></div><div>[... return ...]</div><div><br></div><div>This seems strange to me: rather than having a separate empty() and getNextCorrection(), either of which can terminate the search, could you instead just have getNextCorrection() (which returns the next correction, or an invalid correction if there are no more corrections)?</div>
<div><br></div><div><br></div><div><div>+  ExprResult TransformTypoExpr(TypoExpr *E) {</div></div><div><br></div><div>I think, with the way this currently works, if I have 'f(typo1, typo2)' then I'll try the first correction for typo1 and typo2, then I'll try the second correction for typo1 and typo2, and I'll never try the first correction for typo1 + the second correction for typo2. Is that intentional?</div>
<div><br></div><div><div>+  ExprResult TransformExpr(Expr *E) {</div><div>+    if (trapping)</div><div>+      return realTransformExpr(E);</div><div>+    ExprResult res;</div><div>+    trapping = true;</div><div>+    bool error = false;</div>
<div>+    do {</div><div>+      Sema::SFINAETrap Trap(SemaRef);</div><div>+      res = realTransformExpr(E);</div><div>+      error = Trap.hasErrorOccurred();</div><div>+    } while (error && !TypoExprs.empty());</div>
<div>+    trapping = false;</div><div>+    for (auto E : TypoExprs) {</div><div>+      TypoExpr *TE = cast<TypoExpr>(E);</div><div>+      if (TE->DiagHandler)</div><div>+        (*TE->DiagHandler)(TE->Consumer->getCurrentCorrection());</div>
<div>+    }</div><div>+    for (auto E : FailedTypoExprs) {</div><div>+      TypoExpr *TE = cast<TypoExpr>(E);</div><div>+      if (TE->DiagHandler)</div><div>+        (*TE->DiagHandler)(TE->Consumer->getCurrentCorrection());</div>
<div>+    }</div><div>+    return res;</div><div>+  }</div></div><div><br></div><div>It looks like 'trapping' will only be 'false' here on the outermost expression (that is, the one for which ActOnFinishFullExpr calls TransformExpr). Can you move this logic to ActOnFinishFullExpr instead? Or is this the foundation for something else?</div>
<div><br></div><div><br></div><div>I think we're missing one piece here (though maybe it'll follow later): full expressions and expression evaluation contexts don't necessarily exist in exact correspondence. I think we need to be more careful to:</div>
<div> - keep track of the TypoExprs within each ExprEvaluationContext</div><div> - only remove those ones that ActOnFinishFullExpr has processed, and</div><div> - diagnose any remaining ones when we pop the evaluation context</div>
<br><div class="gmail_quote">On Tue, Jun 17, 2014 at 5:29 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">
<div class=""><div class="h5"><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>
---<br>
 include/clang/Sema/Sema.h         |  25 ++++<br>
 include/clang/Sema/SemaInternal.h |   5 +-<br>
 lib/Sema/SemaExpr.cpp             |   7 +<br>
 lib/Sema/SemaExprCXX.cpp          |  77 ++++++++++<br>
 lib/Sema/SemaLookup.cpp           | 291 ++++++++++++++++++++++++--------------<br>
 5 files changed, 299 insertions(+), 106 deletions(-)<br>
<br>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>