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

[Daniel Dunbar]

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