[cfe-commits] [PATCH] Introduces DynTypedMatcher as a new concept that replaces the UntypedBaseMatcher and TypedMatcher. Due to DynTypedNode the basic dynamically typed matcher interface can now be simplified.

[Manuel Klimek]

On Tue, Sep 4, 2012 at 4:18 AM, Sean Silva <silvas at purdue.edu> 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
> <reviews at llvm-reviews.chandlerc.com> 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