[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/SemaDeclCX

[Doug Gregor]

Hi Chris,

On Wed, Oct 22, 2008 at 5:59 PM, Chris Lattner <clattner at apple.com> wrote:
> 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:

Thanks for the review!

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

Okay.

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

I'd like it to be an array of CXXBaseSpecifiers.

The annoying problem here is that we return CXXBaseSpecifier pointers
to the parser (as a BaseTy*, which is just a void*), when we'd really
prefer to just return a CXXBaseSpecifier. Anyway, I'll switch over to
an array of CXXBaseSpecifiers.

>> +  /// 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?

Yep, sure.

>>
>> +
>> +  /// 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?

No, we don't need random access; I'll switch to an iterator-based interface.

Thanks again for the review.

  - Doug