> /// 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.
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.
--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
