[cfe-dev] C++ PATCH: Basic inheritance syntax, semantics

[Chris Lattner]

On Apr 13, 2008, at 2:02 PM, Doug Gregor wrote:
>> Also, perhaps ParseStructUnionSpecifier/ParseClassSpecifier
>> should be renamed ParseStructUnionClassSpecifier, though that's  
>> pretty long
>> :).  ParseSUCSpecifier?
>
> Yuck. In C++-speak, it's just a "class specifier" because classes,
> structs, and unions are all class types. That's why I prefer the name
> ParseClassSpecifier (but the comment is helpful).

Ok, fair enough!

>> +  AccessSpecifier IsAccessSpecifier();
>>
>> should be 'const'.  Also, for better or worse, other 'is'  
>> predicates in
>> Parser (e.g. isDeclarationSpecifier) use a lowercase 'i', please be
>> consistent with that.  Also, it's somewhat strange for an 'is'  
>> method to
>> return something other than bool.  Maybe this should be:
>> "getAccessSpecifierIfPresent()" or something like that?
>
> I like getAccessSpecifierIfPresent; let's go with that.

Ok.

>> +    // Find the complete source range for the base-specifier.
>> +    // FIXME: Is there a better way to
>> +    SourceRange Range(StartLoc);
>> +    unsigned RawSpecifierEnd
>> +      = Tok.getLocation().getRawEncoding() + Tok.getLength();
>> +     
>> Range.setEnd(SourceLocation::getFromRawEncoding(RawSpecifierEnd));
>>
>> You shouldn't need to do this.  The end location of a source range  
>> actually
>> points to the first character of the last token.
>
> ... and that token is still highlighted with the source range. I
> didn't realize that. Thanks!

np, it is very non-obvious, but is an important simplification for  
clients as you noticed ;-)

> FWIW, the second patch I sent just added the regression test and fixed
> a typo or two. Nothing that requires a separate review.

Ok, sounds great!

-Chris