On Tue, Sep 4, 2012 at 4:18 AM, Sean Silva <[email protected]> wrote:
>> /// Note that we can store \c Decls and \c Stmts by pointer as they are
>> /// guaranteed be unique pointers pointing to dedicated storage in the
>> /// AST. \c QualTypes on the other hand do not have storage or unique
>> /// pointers and thus need to be stored by value.
>> /// Further info: <reference to QualType>
>
> This sounds more like it should be in Clang AST 101, not in the depths
> of the AST matcher implementation. Someone who doesn't know what a
> QualType is isn't going to make it this far into the file; as in all
> writing, considering your audience is important. As such, IMO this
> comment isn't necessary. I think that Manuel's original comment very
> nicely summarizes the status quo as far as "why we're storing the
> QualType by value"; namely, that this is what the AST gives us.
I think the duplication in the docs actually helps, so I went with
Daniel's proposal.
I agree that it also belongs into the Clang AST 101 (will update that
at some point, too).
> However, an explanation is needed here:
>
> /// \brief Supported base node types.
> enum NodeTypeTag {
> NT_Decl,
> NT_Stmt,
> NT_QualType
> } Tag;
>
> , because QualType isn't really a "base node". Since Type/TypeLoc
> _are_ actually bases, an explanation for why and in what sense
> QualType is being used as a "base" here and not Type* or TypeLoc would
> be highly desirable. Or maybe "base" is subtly off from being the
> right concept here, which I think is more along the lines of "a handle
> for a distinct kind of entity in the AST"; it may be easier to just
> say "base" though :P.
If you have a better worded comment proposal, I'm happy to put it in ;)
Thanks!
/Manuel
>
> --Sean Silva
>
> On Mon, Sep 3, 2012 at 4:54 PM, Daniel Jasper
> <[email protected]> wrote:
>>
>>
>> ================
>> Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:36
>> @@ -33,3 +35,3 @@
>> // identifying the matcher) and a pointer to the AST node.
>> typedef std::pair<uint64_t, const void*> UntypedMatchInput;
>>
>> ----------------
>> Manuel Klimek wrote:
>>> Daniel Jasper wrote:
>>> > Add a comment why we don't want memoization on QualTypes (e.g. might be
>>> > incorrect together with the coming hasAncestor matchers or unnecessary as
>>> > there are no big QualType hierarchies).
>>> >
>>> > Also, maybe add FIXME to add memoization to for TypeLocs, once we match
>>> > those (TypeLocs form bigger trees and presumably memoization is worth
>>> > it). Also then it might make sense to store DynTypedNodes instead of
>>> > void*!?
>>> Tried to clarify in the comments.
>>>
>>> Every TypeLoc has a QualType, so I don't see why the second argument holds
>>> - we'll either want to add memoization for both TypeLoc and QualType, or
>>> for neither.
>>>
>>> Using DynTypedNode in the key doesn't make sense at all - we'll never want
>>> to get the original value back, we just need a one-way function that is
>>> unique.
>> Yes, exactly, every TypeLoc has a QualType, so it is sufficient to memoize
>> the TypeLoc (memoizing the QualTypes, too, would at best lead to an
>> improvement by a small constant factor). However, TypeLocs can form longer
>> chains and we don't want to match whole subchains repeatedly for
>> hasDescendant()..
>>
>> "We just need a one-way function that is unique".. This is sort of what
>> DynTypedNode provides (it already knows that for Decls and Stmts the pointer
>> is unique and for QualTypes it has to use the value). Would be good to store
>> information like that in only one class (even though we will never need the
>> retrieval-logic). On the other hand, if you disagree on the first point and
>> say that we will only ever want to memoize Stmts and Decls, this is a mute
>> point ;-).
>>
>> ================
>> Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:96
>> @@ -77,1 +95,3 @@
>> + if (Tag == NT_Decl)
>> + return dyn_cast<T>(*reinterpret_cast<Decl*const*>(Storage));
>> return NULL;
>> ----------------
>> Manuel Klimek wrote:
>>> Daniel Jasper wrote:
>>> > I'd be consistent with getMemoizationData (spacing around * and using
>>> > Storage.buffer).
>>> >
>>> > Actually, how about:
>>> > return dyn_cast<T>(getMemoizationData());
>>> > ?
>>> I dislike using getMemoizationData here. The other way around makes sense.
>>>
>>> Storage.buffer is different, because we get 'Storage' as a const char[] in
>>> this method. Perhaps I should rename it to Buffer to be more consistent?
>> Yes, the other way around makes more sense. I definitely like having only
>> one reinterpret_cast :-).
>>
>> 'Storage' is fine, I just did not read carefully enough, sorry!
>>
>> ================
>> Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:89
>> @@ +88,3 @@
>> + /// Note that we store the QualType by value as we only get
>> + /// it by-value from the AST.
>> + llvm::AlignedCharArrayUnion<Decl*, Stmt*, QualType> Storage;
>> ----------------
>> Manuel Klimek wrote:
>>> Daniel Jasper wrote:
>>> > Some more documentation (or a reference) to what a QualType actually is
>>> > (Type-pointer + 3Bits) might help understanding this easier.
>>> I don't think we currently rely on that - for this implementation, all you
>>> need to know is that QualType doesn't have pointer identity and thus we
>>> need to store it by-value.
>> .. which I would find easier to understand than what the comment currently
>> says.
>>
>> How about:
>>
>> /// Note that we can store \c Decls and \c Stmts by pointer as they are
>> /// guaranteed be unique pointers pointing to dedicated storage in the
>> /// AST. \c QualTypes on the other hand do not have storage or unique
>> /// pointers and thus need to be stored by value.
>> /// Further info: <reference to QualType>
>>
>> (with the "Further info" completely optional).
>>
>>
>> http://llvm-reviews.chandlerc.com/D33
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
