[PATCH] Rename DynTypedNode to ASTNode

[Manuel Klimek]

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