<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 25, 2014 at 1:57 PM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Mon, Jul 14, 2014 at 4:55 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>
<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>
include/clang/AST/DataRecursiveASTVisitor.h | 1 +<br>
include/clang/AST/Expr.h | 20 ++++++++++++++++++++<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 | 16 ++++++++++++++++<br>
lib/Sema/TreeTransform.h | 6 ++++++<br>
lib/Serialization/ASTReaderStmt.cpp | 4 ++++<br>
lib/Serialization/ASTWriterStmt.cpp | 6 ++++++<br>
lib/StaticAnalyzer/Core/ExprEngine.cpp | 1 +<br>
tools/libclang/CXCursor.cpp | 1 +<br>
19 files changed, 80 insertions(+), 1 deletion(-)</blockquote><div><br></div></div></div><div>The AST library is supposed to be self-contained; it's a layering violation to put TypoExpr member definitions into Sema. Conversely, it'd be a layering violation to put them into AST, since they currently delete Sema types. To avoid this, I suggest:</div>
</div></div></div></blockquote><div><br></div><div>I'd have actually preferred to keep TypoExpr out of the AST library altogether since it is a synthetic node generated by Sema for use by Sema and really shouldn't ever be seen/touched outside of Sema, as it is simply a placeholder for Sema's typo correction. But of course defining AST nodes that don't live in the AST library to a certain degree isn't possible... ah well. I knew I'd have to deal with the layering issue at some point</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>* The TypoExpr class should instead store a single opaque 'void*' pointer, and make no attempt to deallocate or otherwise manage it.<br></div><div>* Sema owns the pointee and is responsible for deleting it (you can delete it and zero out the pointer on the TypoExpr at the point when you handle the TypoExpr)</div>
<div><br></div><div>Alternatively, since you need Sema to track the live TypoExprs anyway, you could have Sema maintain a map from TypoExpr* to whatever data you want to store for the TypoExprs, and have the AST node be essentially stateless.</div>
</div></div></div></blockquote><div><br></div><div>I think I'll go with this approach since a TypoExpr node is just a placeholder for internal use by Sema. For the mapping, do you think it would be better to move the recovery callback and diagnostic generator into the TypoCorrectionConsumer so that the map is simply from a TypoExpr to a TypoCorrectionConsumer, or to place the three unique_ptrs in a tuple that would go into the map, or to wrap the three unique_ptrs in a struct that is stored in the map?</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>Incidentally, I wonder whether it'd be useful for TypoExpr to store the typoed identifier, maybe as an IdentifierInfo*? (This'd allow you to produce something more useful from the statement printer and ast dumper, and we really don't care much how big a TypoExpr is, since we'll -- presumably -- never have many of them.) This might also be useful for clients that want to turn off typo correction but keep the TypoExprs in the AST.</div>
</div></div></div>
</blockquote></div><br></div><div class="gmail_extra">As the typo correction is currently structured, a TypoExpr would never be generated when typo correction is turned off since the TypoExpr is only for Sema to know when and where to go back to perform typo correction when it wasn't immediately performed. I'm also not sure how useful it would be to have meaningful output from the statement printer and AST dumper since the TypoExpr shouldn't be kept around that long (it should be gone by the time the full statement has finished being parsed and analysed--I cannot think of any cases where that wouldn't be true that wouldn't also be a bug; after all, until the TypoExpr is handled Sema won't know if the error it represents is recoverable or what the recovered AST should be). If/once there's a good use case for having the identifier-to-be-corrected stored in the TypoExpr, it'll be simple enough to add since it is available when the TypoExpr is created and is also already being stored in the TypoCorrectionConsumer.</div>
</div>