<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, May 30, 2014 at 4:28 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@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, May 30, 2014 at 5:45 AM, 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>On Wed, May 28, 2014 at 12:29 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@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>On Wed, May 28, 2014 at 12:19 AM, 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">This type is a union of:<div><br></div><div> - Stmt</div><div> - Decl</div><div> - Type</div><div> - TemplateArgument</div>
<div> - NestedNameSpecifier</div><div> - NestedNameSpecifierLoc (!)</div><div> - QualType (!)</div>
<div> - TypeLoc (!)</div><div> - CXXCtorInitializer<br><div><br></div><div>... and is used for:</div><div><br></div><div><div class="gmail_extra"> - AST matchers<br> - the parent map (as an implementation detail, where it's really just used as an obscure way of writing PointerUnion<Decl*,Stmt*>)<br>
- and nothing else<br><br>This really seems to be a discriminated union of "the things that AST matchers want to match", rather than some fundamental concept like an "AST node". So, I'd suggest...</div>
</div></div></div></blockquote><div><br></div></div><div>From the AST matchers side "the things that AST matchers want to match" is by definition "all AST nodes".</div><div>(AST matchers also have "attribute matchers" (hasName, etc), but those always operate on a node).</div>
<div><br></div><div>Since you put (!) behind some of them, let me try to explain why I think those are AST nodes. I'd roughly describe an AST node as "a part of the AST that has attributes, or connects other AST nodes". Thus, the "Loc"s are in there because there is no entity of which a TypeLoc is a mere attribute (a TypeLoc is more like a type at a location than a location of a type in that way). Same for QualType, which is more a type with qualifiers than the qualifiers of a type (here the name matches that concept).</div>
</div></div></div></blockquote><div><br></div></div><div>I think what's going on here is that you're trying to put a virtual "AST" graph on top of the actual AST nodes. The nodes and edges in that graph seem to be defined as "what RecursiveASTVisitor visits", and the ParentMap provides the reverse edges in that graph.</div>
</div></div></div></blockquote><div><br></div></div><div>I thought that the idea was that RAV implements visiting the AST.</div><div class=""><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>Then the AST matchers match on that graph, and RAV visits it.</div><div><br></div><div>This is problematic in a few ways (none of which are insurmountable):</div>
<div><br></div><div>- This seems to be a structure that emerged, rather than one that was explicitly designed as a graph on top of the AST, and as a result, we have a semi-incoherent set of nodes in the DynTypedNode union. Why is there no TemplateArgumentLoc? Why are base specifiers missing from the graph? Why are lambda captures missing? Why do we sometimes have nodes with location information and sometimes have nodes without location information? Are we trying to represent the code-as-written or the semantic form?</div>
</div></div></div></blockquote><div><br></div></div><div>The answer to most of the questions is: because nobody has needed it hard enough yet to spend time on implementing it.</div><div>The answer to the last one is: both (well, at least I thought the AST wanted to represent both).</div>
<div class="">
<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>- ParentMap doesn't actually do that. It only maps Decls and Stmts to their parent Decls and Stmts. Perhaps the intent is for ParentMap to represent that graph, but that's not what it does right now.</div><div>
- RAV doesn't define a single unique traversal; it has a bunch of different traversal modes. (This is really the same question as "Are we trying to represent the code-as-written or the semantic form?")</div>
<div><br></div><div>If we want to have this virtual graph on top of the AST, we should consciously make an effort to represent it nicely and be explicit that that's what we're doing. (Right now, it's easier to get to parents of a DynTypedNode than children, at least in the cases where the parent map works.)</div>
</div></div></div></blockquote><div><br></div></div><div>Yep. I agree in general. That said, getting the children of a DynTypedNode (if you know its type) is just as hard as if you have a statically typed node (which is currently in the RAV).</div>
<div class="">
<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><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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div><div><div class="gmail_extra">1) The parent map stops using it and uses a more appropriate data structure (unless we have some reason to think this is the right long-term direction for it?)</div>
</div></div></div></blockquote><div><br></div></div><div>Since generally I think we want to get parents of TypeLocs and the like (because they for a tree that is not accessible in any other way),</div></div></div></div></blockquote>
<div><br></div></div><div>That would be difficult and annoying to support. TypeLocs don't have identity, and don't make good map keys.</div></div></div></div></blockquote><div><br></div></div><div>Depends. While TypeLocs are value objects, they do form a key / identity for questions like "for this BuiltinTypeLoc, what is the ArrayTypeLoc whose element type loc it is?".</div>
<div class="">
<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> But if that's the direction you think the ParentMap should go, I'm not fundamentally opposed. That said, I don't think the map belongs in ASTContext; this virtual graph is a layer on top of the AST rather than part of it.</div>
</div></div></div></blockquote><div><br></div></div><div>I fully agree. I (still) don't know enough about clang to make good decisions here, and usually punt to Doug and you... I don't know where else to put the information though; I don't think we have a good interface that can take ownership of AST-bound layers (yet).</div>
<div class="">
<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><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> I'd guess that either</div>
<div>a) you have a good different idea on how to make a non-statically-typed getParents implementation (in which case I agree with you)</div><div>b) splitting it out would lead to a lot of duplication</div><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><div>
<div class="gmail_extra">2) We move it into ASTMatchers/ as clang::ast_matchers::Node or similar.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Likewise, I think RecursiveASTVisitor should have its own discriminated union type, rather than trying to cram its requirements into DynTypedNode.</div>
</div></div></div></blockquote><div><br></div></div><div>Why? I think DynTypedNode has a pretty good match for everything that tries to traverse the AST in a non-statically-typed way, as that is kind of what it was made for.</div>
</div></div></div></blockquote><div><br></div></div><div>Why are the AST matchers traversing in a non-statically-typed way? Isn't static typing one of their strengths (or do you only get static typing in the interface and not in the implementation)? I would have thought you only need DynTypedNode for handling values bound by matchers and the like. Maybe that's not actually the case?</div>
</div></div></div></blockquote><div><br></div></div><div>You're right that most of the matching process is statically typed, but:</div><div>1. originally we went without a dynamic node, and it led to all kinds of duplication, and distributing the information about each node over all kind of places in the code, which seems bad separation of concerns</div>
<div>2. dynamic matchers, clang-query etc depend on having support for dynamically typed matchers and nodes anyway</div><div>Which is why it turned out that basically taking away the statically typed code at the corners of the interface is the best approach we found so far.</div>
<div><br></div><div>Open questions:<br></div><div>- how do we provide lifetime for things that are layered on top of the AST? currently I don't think we have a good solution apart from putting those things into ASTContext</div>
</div></div></div></blockquote><div><br></div><div>Why can't we just use C++'s structured lifetimes?<br><br></div><div>ASTGraph G(Ctx);<br></div><div>// ... use G<br><br></div><div>-- Sean Silva<br></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>- how do we make traversal more explicit? currently traversal is clearly an implementation detail of each node in the AST, and the RAV just calls that out.</div><div><br></div><div>I think we need to answer those questions before being able to decide what to do (or not to do) with the DynTypedNode and getParents().</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div><div><div class="h5"><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><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>Cheers,</div><div>/Manuel</div><div>
<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><div><div class="gmail_extra"><div><div>
<div class="gmail_quote">On Tue, May 20, 2014 at 11:06 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@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 dir="ltr">+doug & richard, as the keepers of clang<div><br></div><div>I'm not too bought into any name, but somebody needs to make a decision.</div></div><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote">
On Tue, May 20, 2014 at 8:00 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.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 dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div>On Tue, May 20, 2014 at 11:50 AM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.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>
On 20/05/2014 20:42, Sean Silva 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>
<br>
<br>
<br>
On Tue, May 20, 2014 at 11:37 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
<br>
On 20/05/2014 20:34, Manuel Klimek wrote:<br>
<br>
On Tue, May 20, 2014 at 7:19 PM, Sean Silva<br>
<<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a> <mailto:<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>><br></div>
<mailto:<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a> <mailto:<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>><u></u>>><div>
<br>
wrote:<br>
<br>
Manuel, do you remember why this name was chosen? I seem<br>
to recall<br>
thinking that the name was well-chosen.<br>
<br>
<br>
It's a DynTypedNode, because the type information is kept /<br>
available dynamically. ASTNode would suggest that there is a<br>
common interface for AST nodes, which there is not.<br>
<br>
<br>
Well, QualType is a wrapper around Type structures that works<br>
similarly and we don't make the distinction there.<br>
<br>
<br>
What doesn't work for me is saying "DynTypedNode is an AST<br>
node". This feels wrong. DynTypedNode is more like a "smart<br>
reference".<br>
<br>
<br>
How about ASTNodeRef? I don't have a strong feeling about ASTNode<br>
/ ASTNodeRef or something along those lines, but DynTypedNode<br>
specifically doesn't feel right.<br>
<br>
The current name doesn't mean much outside ASTMatchers lingo<br>
whereas the functionality is fairly general and could be useful<br>
elsewhere.<br>
<br>
<br>
There was some speculation about extending use of DynTypedNode outside the ASTMatchers library <a href="http://reviews.llvm.org/D33?id=85#inline-386" target="_blank">http://reviews.llvm.org/D33?<u></u>id=85#inline-386</a>; is there something in particular that you have in mind that you are trying to work towards?<br>
</div></blockquote>
<br>
Yes, it became apparent during the RecursiveASTVisitor / DataRecursiveASTVisitor that we have two data recursion techniques and this showed up as a possible way forward while investigating whether it's feasible to unify them. That kind of unification would be far off but the associated cleanup makes sense to make this class more discoverable outside of ASTMatchers.<br>
<br>
DynTypedNode, along with the parent map in ASTContext is fairly useful with the caveat that the code reads rather silly with the name "DynTypedNode" where what you have and what you want is an AST node reference.<br>
</blockquote><div><br></div></div></div><div>Makes sense; I agree that DynTypedNode doesn't work too well in that context.</div><div><br></div><div>I'm still not a fan of anything that would suggest that there is a unified AST hierarchy (IMO even ASTNodeRef suggests that there is some sort of underlying unified "ASTNode" being "Ref"erenced). Maybe ASTVariant?</div>
<span><font color="#888888">
<div><br></div><div>-- Sean Silva</div></font></span><div><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">
<br>
Alp.<br>
<br>
<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>
<br>
-- Sean Silva<br>
<br>
<br>
Alp.<br>
<br>
<br>
<br>
<br>
Cheers,<br>
/Manuel<br>
<br>
<br>
-- Sean Silva<br>
<br>
<br>
On Sun, May 18, 2014 at 11:18 PM, Alp Toker<br>
<<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>><br></div><div>
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>>> wrote:<br>
<br>
This is always referred to as an AST node in<br>
documentation. By<br>
wrapping Decls, Stmts and QualTypes it provides a<br>
generalised<br>
way to reference AST nodes. And despite the name,<br>
DynTypedNode<br>
doesn't have anything to do with dynamic types in the AST.<br>
<br>
The attached patch renames the class and implementation to<br>
ASTNode.<br>
<br>
(Indeed the namespace "ast_type_traits" is equally<br>
confusing.<br>
Presumably both are cases where implementation details<br>
have<br>
leaked into the naming scheme?)<br>
<br>
Alp.<br>
<br>
-- <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
<br>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><br></div>
<mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>>><div>
<br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
<br>
<br>
<br>
<br>
-- <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
<br>
</div></blockquote><div><div>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>