It can be done in a later patch. For now, the current patch LGTM. --Sean Silva
On Tue, Sep 4, 2012 at 1:28 AM, Manuel Klimek <[email protected]> wrote: > 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
