<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jun 27, 2014 at 8:05 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</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"><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></blockquote><div><br></div><div>*sigh* missed that one. /me wishes vim had better support for doxygen-style comments.</div>
<div> </div><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"><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></blockquote><div><br></div>
<div>Deleted this entirely; see below.</div><div> </div><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">
<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></div></blockquote><div><br></div><div>*sigh* you're right--I don't know why I did that since getNextCorrection() returns an empty correction if there are no more available. Changed the while+if to simply be "while (TypoCorrection TC = E->Consumer->getNextCorrection()) {"</div>
<div><br></div><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"><div class="gmail_extra">
<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></div></blockquote><div><br></div><div>It wasn't intentional--I already knew of this shortcoming but was having trouble figuring out how to implement the necessary backtracking. A little while ago I had a flash of inspiration and realized handling this case would be fairly easy if I change the TypoCorrectionConsumer to store *all* of the corrections returned by getNextCorrection (in order) instead of just the last one and add a method to "reset" the TypoCorrectionConsumer such that getNextCorrection would re-step through the previously returned corrections before looking for a new valid correction (i.e. make getNextCorrection replayable). I shall implement that as a replacement for patch #4 "Have TypoCorrectionConsumer remember the last TypoCorrection returned." in v3 of the patch set, though the set does not encounter any cases in the unit tests that run afoul of the current limitation.</div>
<div><br></div><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"><div class="gmail_extra">
<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></div></blockquote><div><br></div><div>Ack, you're right. I've renamed TransformExpr to Transform (I'm open to better entry-point-ish name suggestions) and realTransformExpr to TransformExpr since all of the logic in the old TransformExpr is only used when initially kicking off this TreeTransform. I also deleted 'trapping' entirely since it's no longer used.</div>
<div><br></div><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"><div class="gmail_extra">
<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></div></blockquote><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"><div class="gmail_extra">
<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>
</div></div></blockquote><div><br></div><div>Yeah, I've discovered the lack of correspondence between full expressions and expression evaluation contexts. When I went back to trying to wire up CorrectTypoDelayed to DiagnoseEmptyLookup for use by a single caller of DiagnoseEmptyLookup (specifically, ActOnIdExpression) after mailing out this patch set, I encountered a number of situations where a TypoExpr was created and the count in an ExprEvaluationContext was updated but ActOnFinishFullExpr was never called for the Expr (in addition to the cases I already mentioned where the typo count has to be propagated up the stack of expression evaluation contexts). I suspect that handling the remaining TypoExprs in an evaluation context when the context is popped will diagnose the TypoExpr too soon in some cases (even when it should have and otherwise would have been handled in ActOnFinishFullExpr). And unless I've missed something about the state accessible from PopExpressionEvaluationContext, finding the statements that contain the remaining TypoExprs to be able to replace the TypoExprs would be extremely difficult since Exprs don't contain back-links to their parents. I'm certainly open to better ideas for tracking when an Expr may have a TypoExpr within it and needs the tree transform run on it--right now the typo count in the ExprEvaluationContext is just being used as a flag to have an idea of an Expr should have the tree transform run on it to deal with TypoExprs (even with the tree transform being applied from places besides ActOnFinishFullExpr, such as dealing with the inline initializer Expr for a class member within the class' declaration).</div>
<div><br></div>
<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"><div class="gmail_extra">
<br><div class="gmail_quote"><div><div>On Tue, Jun 17, 2014 at 5:29 PM, 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>
</div></div><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><div>
<div><div><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></div></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=cfe-commits@cs.uiuc.edu&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" rel="noreferrer" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>
</blockquote></div><br></div></div>