[PATCH] Rename DynTypedNode to ASTNode

[Sean Silva]

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/20140520/cc49c67f/attachment.html>