[PATCH] Rename DynTypedNode to ASTNode
Alp Toker
alp at nuanti.com
Tue May 27 17:30:57 PDT 2014
On 28/05/2014 01:19, Richard Smith 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".
It's more interesting than that:
The AST matcher entities are a near subset of the kinds of nodes a
CXCursor represents in libclang.
AST matchers and libclang both use RAVs and both have a similar concept
of parenting.
So although the two were developed at different times by separate teams,
there's commonality here that's been missed.
> So, I'd suggest...
>
> 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?)
No strong feeling on the parent map, except that we could probably make
more use of it (it's a plugin/tooling developer FAQ on the list).
> 2) We move it into ASTMatchers/ as clang::ast_matchers::Node or similar.
It's not the prettiest union but if we can clean it up on the way (or
devise something better) and bring the goodness along to AST matchers
and/or libclang I'm all for that. Should evaluate potential for reuse
before shuffling it away.
>
> Likewise, I think RecursiveASTVisitor should have its own
> discriminated union type, rather than trying to cram its requirements
> into DynTypedNode.
On the other hand, if we have multiple consumers with such similar
requirements and code, perhaps that cramming is a feature.
I like the idea that we can share code here and even edge towards having
user-facing facilities like matchers and API present views on the same
hierarchy, instead of slightly different ones. The course of action
isn't clear yet though so this is a good discussion.
Alp.
>
> On Tue, May 20, 2014 at 11:06 AM, Manuel Klimek <klimek at google.com
> <mailto: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
> <mailto:chisophugis at gmail.com>> wrote:
>
>
>
>
> On Tue, May 20, 2014 at 11:50 AM, Alp Toker <alp at nuanti.com
> <mailto: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>
> <mailto: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>>
> <mailto: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>>
> <mailto: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>>
> <mailto: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
>
>
>
>
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list