<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 25, 2014 at 3:21 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: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="">On Fri, Jul 25, 2014 at 1:57 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">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>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>
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><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 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 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><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></div></div></blockquote><div><br></div><div>Since the map will be internal to Sema, I think you should go with whatever option makes the Sema implementation most convenient. Since we use TypoCorrectionConsumer for the non-delayed case, keeping the other parts separate seems likely to be best.</div>
<div> </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=""><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></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>
</blockquote></div><br></div><div class="gmail_extra">OK, I'm happy keeping it empty for now. I asked because we have several clang-as-a-library users who've asked for both</div><div class="gmail_extra">a) parse what you can, and build an approximate AST for what you can't, and</div>
<div class="gmail_extra">b) tell me where the typos are, but don't actually correct them</div><div class="gmail_extra">... and it seems that we could improve the situation in both cases if we had a mode to create TypoExprs but not act on them. Let's keep that as a separate discussion to be had once this work lands, though.</div>
</div>