<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jun 27, 2014 at 10:42 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class=""><div class="gmail_quote">On Fri, Jun 27, 2014 at 7:31 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>
</div></div><div class="gmail_quote"><div class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>+ TypoExpr(std::unique_ptr<TypoCorrectionConsumer> TCC,</div>
<div>+ std::unique_ptr<TypoDiagnosticGenerator> TDG);</div><div>+ child_range children() { return child_range(); }</div>
<div>+ SourceLocation getLocStart() const LLVM_READONLY { return SourceLocation(); }</div><div>+ SourceLocation getLocEnd() const LLVM_READONLY { return SourceLocation(); }</div><div>+</div><div>+ std::unique_ptr<TypoCorrectionConsumer> Consumer;</div>
<div>+ std::unique_ptr<TypoDiagnosticGenerator> DiagHandler;</div><div><br></div><div>These unique_ptrs will be leaked; AST nodes don't get destroyed. Please allocate your handlers on the ASTContext instead.</div>
</div></blockquote><div><br></div></div><div>Well @#$%. I'm assuming that allocating these on the ASTContext would mean calling e.g. "new (SemaRef.Context) TypoCorrectionConsumer(...)" instead of "llvm::make_unique<TypoCorrectionConsumer>(...)" and passing around raw pointers instead of unique_ptrs?</div>
</div></div></div></blockquote><div>ping ^^^^ <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div class="">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div><br></div><div><div>+TypoExpr::TypoExpr(std::unique_ptr<TypoCorrectionConsumer> TCC,</div><div>+ std::unique_ptr<TypoDiagnosticGenerator> TDG)</div><div>+ : Expr(TypoExprClass, TCC->getContext().DependentTy, VK_LValue, OK_Ordinary,</div>
<div>+ /*isTypeDependent*/ true,</div><div>+ /*isValueDependent*/ true,</div><div>+ /*isInstantiationDependent*/ true,</div><div>+ /*containsUnexpandedParameterPack*/ false),</div>
</div>
<div><br></div><div>Hmm, parameter packs are an interesting twist on this. Suppose we have:</div><div><br></div><div> template<typename...Thing> void f(Thing ...thing) { g(rhing...); }</div><div><br></div><div>if we set containsUnexpandedParameterPack to false, we'll reject this in the initial parse. If we set it to true, we'll reject other stuff instead. =( Maybe we should suppress diagnosing an unexpanded pack if we've got an active typo correction?</div>
</div></blockquote><div><br></div></div><div>Umm... yeah... having not really worked with parameter packs or looked at any (failing) typo-correction unit tests involving parameter packs yet, I don't know what the right answer is. Nor do I quite know where the boundary of containsUnexpandedParameterPack being true would fall relative to an expression placeholder that represents a possibly-qualified identifier and the hooks needed to expand the identifier into an expression that fits in where the identifier was seen.</div>
<div class="">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div><br></div><div><div> template<typename Derived></div><div> ExprResult</div><div>+TreeTransform<Derived>::TransformTypoExpr(TypoExpr *E) {</div><div>+ assert(getDerived().AlreadyTransformed(E->getType()) &&</div>
<div>+ "typo-correction placeholder expression requires transformation");</div><div>+ return Owned(E);</div><div>+}</div></div><div><br></div><div>Can we really not get here with a TypoExpr still in the tree? We have enough TreeTransforms these days that I find this a bit surprising. Maybe TreeTransform should just preserve TypoExprs by default.</div>
</div></blockquote><div><br></div></div><div>D'oh, you're right. When I added all of the (Transform|Visit)TypoExpr methods to the various classes like this one and ASTStmtReader in order to make the compiler happy, I applied pretty much the same code or mentality to all of the methods: "a TypoExpr is internal to Sema so shouldn't ever hit these paths since it should never still be in the AST tree when the tree is serialized, deserialized, etc)". I've dropped the assert here (and the now-defunct call to Owned).</div>
<div class="">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div><br></div><div><div>+void ASTStmtReader::VisitTypoExpr(TypoExpr *E) {</div><div>+ VisitExpr(E);</div><div>+ // TODO: Figure out sane reader behavior for a TypoExpr, if necessary.</div><div>+ assert(false && "Cannot read TypoExpr nodes");</div>
<div>+}</div></div><div><br></div><div>I think this should be unreachable; if things have gone so badly wrong that we couldn't resolve a TypoExpr, we shouldn't serialize it either.</div></div></blockquote><div><br>
</div></div><div>I've replaced the three lines in the body with a call to llvm_unreachable().</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra">
<br><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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
<div><div>---<br>
include/clang/AST/DataRecursiveASTVisitor.h | 1 +<br>
include/clang/AST/Expr.h | 17 +++++++++++++++++<br>
include/clang/AST/RecursiveASTVisitor.h | 1 +<br>
include/clang/Basic/StmtNodes.td | 1 +<br>
include/clang/Sema/SemaInternal.h | 3 +++<br>
include/clang/Sema/TypoCorrection.h | 5 +++++<br>
lib/AST/Expr.cpp | 1 +<br>
lib/AST/ExprClassification.cpp | 3 ++-<br>
lib/AST/ExprConstant.cpp | 1 +<br>
lib/AST/ItaniumMangle.cpp | 1 +<br>
lib/AST/StmtPrinter.cpp | 5 +++++<br>
lib/AST/StmtProfile.cpp | 4 ++++<br>
lib/Sema/SemaExceptionSpec.cpp | 1 +<br>
lib/Sema/SemaLookup.cpp | 11 +++++++++++<br>
lib/Sema/TreeTransform.h | 8 ++++++++<br>
lib/Serialization/ASTReaderStmt.cpp | 6 ++++++<br>
lib/Serialization/ASTWriterStmt.cpp | 6 ++++++<br>
lib/StaticAnalyzer/Core/ExprEngine.cpp | 1 +<br>
tools/libclang/CXCursor.cpp | 1 +<br>
19 files changed, 76 insertions(+), 1 deletion(-)<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" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>