[cfe-commits] r57991 - in /cfe/trunk: include/clang/AST/DeclCXX.h include/clang/Basic/DiagnosticKinds.def include/clang/Parse/Action.h include/clang/Parse/Parser.h lib/AST/DeclCXX.cpp lib/Parse/ParseDeclCXX.cpp lib/Sema/Sema.h lib/Sema/SemaDeclCXX.cpp lib/Sema/SemaOverload.cpp test/SemaCXX/inherit.cpp

[Chris Lattner]

On Oct 22, 2008, at 10:49 AM, Douglas Gregor wrote:
> Author: dgregor
> Date: Wed Oct 22 12:49:05 2008
> New Revision: 57991
>
> URL: http://llvm.org/viewvc/llvm-project?rev=57991&view=rev
> Log:
> Add representation of base classes in the AST, and verify that we
> don't have duplicated direct base classes.

Nice, some picky stuff:

> +++ cfe/trunk/include/clang/AST/DeclCXX.h Wed Oct 22 12:49:05 2008
> @@ -40,17 +40,113 @@
>   static bool classof(const CXXFieldDecl *D) { return true; }
> };
>
> +/// BaseSpecifier - A base class of a C++ class.
> +class CXXBaseSpecifier {

Please give an example of one in this comment.  This is useful for  
people who aren't experts in the language.  For example, something  
silly like this is sufficient:

///  In "class foo : protected virtual bar, private zug" there are two
/// CXXBaseSpecifiers, one for "protected virtual bar" and one for  
"private zug".

This sort of thing is very useful to me in the ObjC frontend (because  
I barely know ObjC) so I'm sure it will be helpful for those who  
aren't experts in C++.

>
> +  SourceRange Range;
> +
> +  bool Virtual : 1;

What is this field? ;-)

> +public:
> +  static CXXBaseSpecifier *Create(ASTContext &C, SourceRange R,  
> bool V, bool BC,
> +                                  AccessSpecifier A, QualType T);

you're making an array of pointers to CXXBaseSpecifier's.  Would it be  
better to just have an array of CXXBaseSpecifier's directly?   
Alternatively, instead of an array of pointers, maybe a linked list of  
CXXBaseSpecifier's would be better?  If you prefer to stay with an  
array, maybe "ObjCList" (see the top of DeclObjC.h) would work better  
and should be generalized.


> class CXXRecordDecl : public RecordDecl, public DeclContext {
...
>
> public:
>   static CXXRecordDecl *Create(ASTContext &C, TagKind TK,  
> DeclContext *DC,
>                                SourceLocation L, IdentifierInfo *Id,
>                                CXXRecordDecl* PrevDecl=0);
>
> +  /// setBases - Sets the base classes of this struct or class.
> +  void setBases(CXXBaseSpecifier **Bases, unsigned NumBases) {
> +    this->Bases = Bases;
> +    this->NumBases = NumBases;
> +  }

This is the only serious request I have with this patch: please make  
"setBases" allocate the memory to hold the array (if you stick with an  
array).  To keep the ownership policy simple, I'd prefer to stay away  
from cases where the caller allocates memory for the data structure  
and transfers it into the AST node.  Historically, the ObjC decl  
classes were a big offender in this area and were a big source of bugs.

I'd prefer to follow a model like the various ObjC classes are using  
now.  For example, ObjCCategoryDecl has stuff like this:

   /// addReferencedProtocols - Set the list of protocols that this  
interface
   /// implements.
   void addReferencedProtocols(ObjCProtocolDecl *const*List, unsigned  
NumRPs) {
     ReferencedProtocols.set(List, NumRPs);
   }

This takes ownership of the ObjCProtocolDecl's themselves, but does  
not take ownership of the array of pointers passed in.  In practice,  
the array of pointers passed in is built up as a SmallVector and then  
copied out by addReferencedProtocols.

Does this make sense?

>
> +
> +  /// getNumBases - Retrieves the number of base classes of this
> +  /// class.
> +  unsigned getNumBases() const { return NumBases; }
> +
> +  /// getBase - Retrieve the ith base class.
> +  CXXBaseSpecifier *getBase(unsigned i) {
> +    assert(i < NumBases && "Base index out of range");
> +    return Bases[i];
> +  }
> +
> +  /// getBase - Retrieve the ith base class.
> +  const CXXBaseSpecifier *getBase(unsigned i) const {
> +    assert(i < NumBases && "Base index out of range");
> +    return Bases[i];
> +  }

How about an iterator interface?  Do you anticipate needing random  
access to numbered bases?

Thanks Doug!

-Chris