[PATCH] Rename DynTypedNode to ASTNode
Sean Silva
chisophugis at gmail.com
Fri May 30 14:12:54 PDT 2014
On Fri, May 30, 2014 at 4:28 AM, Manuel Klimek <klimek at google.com> wrote:
> On Fri, May 30, 2014 at 5:45 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On Wed, May 28, 2014 at 12:29 AM, Manuel Klimek <klimek at google.com>
>> wrote:
>>
>>> On Wed, May 28, 2014 at 12:19 AM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>
>>>> This type is a union of:
>>>>
>>>> - Stmt
>>>> - Decl
>>>> - Type
>>>> - TemplateArgument
>>>> - NestedNameSpecifier
>>>> - NestedNameSpecifierLoc (!)
>>>> - QualType (!)
>>>> - TypeLoc (!)
>>>> - CXXCtorInitializer
>>>>
>>>> ... and is used for:
>>>>
>>>> - AST matchers
>>>> - the parent map (as an implementation detail, where it's really just
>>>> used as an obscure way of writing PointerUnion<Decl*,Stmt*>)
>>>> - and nothing else
>>>>
>>>> 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...
>>>>
>>>
>>> From the AST matchers side "the things that AST matchers want to match"
>>> is by definition "all AST nodes".
>>> (AST matchers also have "attribute matchers" (hasName, etc), but those
>>> always operate on a node).
>>>
>>> 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).
>>>
>>
>> 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.
>>
>
> I thought that the idea was that RAV implements visiting the AST.
>
>
>> Then the AST matchers match on that graph, and RAV visits it.
>>
>> This is problematic in a few ways (none of which are insurmountable):
>>
>> - 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?
>>
>
> The answer to most of the questions is: because nobody has needed it hard
> enough yet to spend time on implementing it.
> The answer to the last one is: both (well, at least I thought the AST
> wanted to represent both).
>
>
>> - 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.
>> - 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?")
>>
>> 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.)
>>
>
> 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).
>
>
>> 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?)
>>>>
>>>
>>> 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),
>>>
>>
>> That would be difficult and annoying to support. TypeLocs don't have
>> identity, and don't make good map keys.
>>
>
> 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?".
>
>
>> 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.
>>
>
> 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).
>
>
>> I'd guess that either
>>> a) you have a good different idea on how to make a non-statically-typed
>>> getParents implementation (in which case I agree with you)
>>> b) splitting it out would lead to a lot of duplication
>>>
>>>
>>>> 2) We move it into ASTMatchers/ as clang::ast_matchers::Node or
>>>> similar.
>>>>
>>>> Likewise, I think RecursiveASTVisitor should have its own discriminated
>>>> union type, rather than trying to cram its requirements into DynTypedNode.
>>>>
>>>
>>> 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.
>>>
>>
>> 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?
>>
>
> You're right that most of the matching process is statically typed, but:
> 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
> 2. dynamic matchers, clang-query etc depend on having support for
> dynamically typed matchers and nodes anyway
> 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.
>
> Open questions:
> - 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
>
Why can't we just use C++'s structured lifetimes?
ASTGraph G(Ctx);
// ... use G
-- Sean Silva
> - 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.
>
> 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().
>
> Cheers,
> /Manuel
>
>
>>
>> Cheers,
>>> /Manuel
>>>
>>>
>>>> On Tue, May 20, 2014 at 11:06 AM, Manuel Klimek <klimek at google.com>
>>>> wrote:
>>>>
>>>>> +doug & richard, as the keepers of clang
>>>>>
>>>>> I'm not too bought into any name, but somebody needs to make a
>>>>> decision.
>>>>>
>>>>>
>>>>> On Tue, May 20, 2014 at 8:00 PM, Sean Silva <chisophugis at gmail.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, May 20, 2014 at 11:50 AM, Alp Toker <alp at nuanti.com> wrote:
>>>>>>
>>>>>>>
>>>>>>> On 20/05/2014 20:42, Sean Silva wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, May 20, 2014 at 11:37 AM, Alp Toker <alp at nuanti.com
>>>>>>>> <mailto:alp at nuanti.com>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 20/05/2014 20:34, Manuel Klimek wrote:
>>>>>>>>
>>>>>>>> On Tue, May 20, 2014 at 7:19 PM, Sean Silva
>>>>>>>> <chisophugis at gmail.com <mailto:chisophugis at gmail.com>
>>>>>>>> <mailto:chisophugis at gmail.com <mailto:chisophugis at gmail.com
>>>>>>>> >>>
>>>>>>>>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Manuel, do you remember why this name was chosen? I seem
>>>>>>>> to recall
>>>>>>>> thinking that the name was well-chosen.
>>>>>>>>
>>>>>>>>
>>>>>>>> It's a DynTypedNode, because the type information is kept /
>>>>>>>> available dynamically. ASTNode would suggest that there is a
>>>>>>>> common interface for AST nodes, which there is not.
>>>>>>>>
>>>>>>>>
>>>>>>>> Well, QualType is a wrapper around Type structures that works
>>>>>>>> similarly and we don't make the distinction there.
>>>>>>>>
>>>>>>>>
>>>>>>>> What doesn't work for me is saying "DynTypedNode is an AST
>>>>>>>> node". This feels wrong. DynTypedNode is more like a "smart
>>>>>>>> reference".
>>>>>>>>
>>>>>>>>
>>>>>>>> How about ASTNodeRef? I don't have a strong feeling about
>>>>>>>> ASTNode
>>>>>>>> / ASTNodeRef or something along those lines, but DynTypedNode
>>>>>>>> specifically doesn't feel right.
>>>>>>>>
>>>>>>>> The current name doesn't mean much outside ASTMatchers lingo
>>>>>>>> whereas the functionality is fairly general and could be useful
>>>>>>>> elsewhere.
>>>>>>>>
>>>>>>>>
>>>>>>>> There was some speculation about extending use of DynTypedNode
>>>>>>>> outside the ASTMatchers library http://reviews.llvm.org/D33?
>>>>>>>> id=85#inline-386; is there something in particular that you have
>>>>>>>> in mind that you are trying to work towards?
>>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>
>>>>>> Makes sense; I agree that DynTypedNode doesn't work too well in that
>>>>>> context.
>>>>>>
>>>>>> 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?
>>>>>>
>>>>>> -- Sean Silva
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Alp.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -- Sean Silva
>>>>>>>>
>>>>>>>>
>>>>>>>> Alp.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> /Manuel
>>>>>>>>
>>>>>>>>
>>>>>>>> -- Sean Silva
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sun, May 18, 2014 at 11:18 PM, Alp Toker
>>>>>>>> <alp at nuanti.com <mailto:alp at nuanti.com>
>>>>>>>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>> wrote:
>>>>>>>>
>>>>>>>> This is always referred to as an AST node in
>>>>>>>> documentation. By
>>>>>>>> wrapping Decls, Stmts and QualTypes it provides a
>>>>>>>> generalised
>>>>>>>> way to reference AST nodes. And despite the name,
>>>>>>>> DynTypedNode
>>>>>>>> doesn't have anything to do with dynamic types in
>>>>>>>> the AST.
>>>>>>>>
>>>>>>>> The attached patch renames the class and
>>>>>>>> implementation to
>>>>>>>> ASTNode.
>>>>>>>>
>>>>>>>> (Indeed the namespace "ast_type_traits" is equally
>>>>>>>> confusing.
>>>>>>>> Presumably both are cases where implementation
>>>>>>>> details
>>>>>>>> have
>>>>>>>> leaked into the naming scheme?)
>>>>>>>>
>>>>>>>> Alp.
>>>>>>>>
>>>>>>>> -- http://www.nuanti.com
>>>>>>>> the browser experts
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> cfe-commits mailing list
>>>>>>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>>>>>>> <mailto:cfe-commits at cs.uiuc.edu <mailto:
>>>>>>>> cfe-commits at cs.uiuc.edu>>
>>>>>>>>
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> -- http://www.nuanti.com
>>>>>>>> the browser experts
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> --
>>>>>>> http://www.nuanti.com
>>>>>>> the browser experts
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140530/ededb435/attachment.html>
More information about the cfe-commits
mailing list