[cfe-commits] r56611 - in /cfe/trunk: include/clang/AST/DeclGroup.h lib/AST/DeclGroup.cpp

[Ted Kremenek]

Daniel:

My meta-level comment is that DeclGroup is meant to just be used by  
DeclGroupRef.  Perhaps I should just make it a private implementation  
within DeclGroup.cpp.  From that perspective, whether or not we use  
operator[] is purely for the convenience of DeclGroupRef, and not part  
of the public interface.

Ted

On Sep 25, 2008, at 10:46 AM, Daniel Dunbar wrote:

> Cool! Some comments:
>
> On Thu, Sep 25, 2008 at 10:13 AM, Ted Kremenek <kremenek at apple.com>  
> wrote:
>> +class DeclGroup {
>> ...
>> +  Decl*& operator[](unsigned i) {
>> +    assert (i < NumDecls && "Out-of-bounds access.");
>> +    return *((Decl**) (this+1));
>> +  }
>
> I'm still of the opinion that memory usage in the multiple  
> declarator case is
> a non-issue. I see no problem with just having a vector instead of  
> bothering
> with doing custom allocation.
>
> Also, we don't use overloaded [] for many collections, are you sure  
> we want
> to here as opposed to providing ->getDecl()? I personally think the
> later is more
> transparent, especially since DeclGroupRef will be forwarding  
> methods and
> forwarding operator overloads might obscure the code even more.
>
>> +  iterator begin() {
>> ...
>> +    DeclGroup* G = reinterpret_cast<DeclGroup*>(ThePtr & ~Mask);
>> +    return &(*G)[0];
>> +  }
>> ...
>> +  iterator end() {
>> ...
>> +    DeclGroup* G = reinterpret_cast<DeclGroup*>(ThePtr & ~Mask);
>> +    return &(*G)[0] + G->size();
>> +  }
>
> Again I'd prefer DeclGroup to have begin/end here and just forward  
> to that.
> It may make sense for DeclGroupRef to have
>
> /// asDeclGroup - Return the contained DeclGroup, if any. This will be
> /// null for "packed" DeclGroups.
> DeclGroup *asDeclGroup();
> /// asDecl - Return the contained Decl, if any. This will be
> /// null for non-"packed" DeclGroups.
> Decl *asDecl();
>
> Almost all DeclGroupRef methods should then follow the pattern:
> T foo(... args ...) {
>  if (DeclGroup *DG = asDeclGroup())
>    return DG->foo(... args ...);
>  Decl *D = asDecl();
>  ... implement foo semantics for a single decl ...
> }
>
> This may only be worth it if those methods are exposed, which we
> may very conceivably want.
>
>> +DeclGroup* DeclGroup::Create(ASTContext& C, unsigned numdecls,  
>> Decl** decls) {
>
> s/numdecls/numDecls/g?
>
> - Daniel