[cfe-commits] r58817 - in /cfe/trunk: include/clang/AST/ include/clang/Basic/ include/clang/Parse/ lib/AST/ lib/Basic/ lib/Parse/ lib/Sema/ test/SemaCXX/ www/

[Chris Lattner]

On Nov 17, 2008, at 8:17 AM, Doug Gregor wrote:
>> The fact that it is non-member should be clear from the context  
>> printed with
>> the diagnostic (yay for good location info).  I think "overloaded  
>> operator
>> 'operator*'" is somewhat redundant (if you agree, please change the  
>> other
>> diags as well to be consistent).  The mention of reference types is  
>> pedantic
>> and correct, but I don't think it is necessary to get the point  
>> across.
>> What do you think?
>
> Sure, but I just want to say that I was *very* proud to have snuck
> "thereof" into a compiler error message.

Hehe :)

>> Doing this with DeclarationName eliminates the need to populate and  
>> look
>> these up.  The DeclarationName would just be "I'm an overloaded  
>> operator, of
>> form OO_Plus".  The getName() method would return "operator+" for it.
>
> Yeah, I was thinking about this while writing up the documentation on
> DeclarationName. DeclarationNameExtra::ExtraKind is going to end up
> getting a bunch of new values, and this change will have to come
> *after* we teach IdentifierResolver to work with DeclarationNames. I
> think the end result will be better, though: IdentifierInfo can go
> back to just dealing with identifiers and keywords, and
> DeclarationName will encompass all kinds of names.

Ok, makes sense.  I don't have any feeling on the relative ordering of  
these tasks, and there is no specific urgency to make the change.

>>> +  // C++ [over.oper]p6:
>>> +  //   An operator function shall either be a non-static member
>>> +  //   function or be a non-member function and have at least one
>>> +  //   parameter whose type is a class, a reference to a class, an
>>> +  //   enumeration, or a reference to an enumeration.
>>> +  CXXMethodDecl *MethodDecl = dyn_cast<CXXMethodDecl>(FnDecl);
>>> +  if (MethodDecl) {
>>
>> I see that you use MethodDecl as a predicate later in the  
>> function.  As a
>> reader, it actually helps me understand the code if you put the  
>> dyncast in
>> the if condition, and use "isa<CXXMethodDecl>(FnDecl)" later in the  
>> method
>> for the predicate cases.  This is just fewer variables to keep  
>> track of.
>> Does that seem reasonable?
>
> Hrm, I guess so. Is isa<> relatively cheap? (I guess this isn't a hot
> path, but...).

isa<> (and dyn_cast for that matter) boil down to running the  
'classof' method on the object.  For most classes, this boils down to  
loading the kind and comparing against a constant.  This is  
dramatically cheaper than dynamic_cast.

>>> Is there a reason to do any of the rest of the checking here?  Why  
>>> not just
>> return true?  Here and in other places you continue after emitting  
>> the first
>> diagnostic.  While I do agree about trying to recover after errors,  
>> I think
>> that this routine might lean too much in the direction of causing a  
>> big
>> chain of nonsensical gibberish after a simple error.  Do you think  
>> it would
>> be better to exit immediately after the first error (which would  
>> mean we'd
>> emit at most one diagnostic per operator)?  What are the tradeoffs?
>
> In this routine, it's okay to exit early rather than continue emitting
> errors after the first error. In other places (e.g.,
> CheckDestructorDeclarator), we do some normalization of the declarator
> that should always happen to prevent future crashes. This
> normalization isn't needed with overloaded operators.

Ok.  Thanks for making the change, I think it makes it easier to read  
and reason about the code.

>>>
>>> +    // We have the wrong number of parameters.
>>> +    std::string NumParamsStr = (llvm::APSInt(32) =
>>> NumParams).toString(10);
>>
>> This is scarily clever :), but please just use  
>> llvm::utostr(NumParams).  It
>> is in llvm/ADT/StringExtras.h
>
> Such overly clever code means that I couldn't find llvm/ADT/ 
> StringExtras.h :)

Hehe, fair enough! :)

>
>>> +  // Overloaded operators cannot be variadic.
>>> +  if (FnDecl->getType()->getAsFunctionTypeProto()->isVariadic()) {
>>
>> Huh, operator() can't be variadic?  I didn't realize that, but then  
>> again, I
>> guess I never had need to try :)
>
> Argh! Good catch. operator() can be variadic, because it's an
> exception to the rules in this section.

Whoa, ok :)

>> A source range would be better suited when emitting
>> errors about parameters to the operator (underlying the parameter in
>> question).
>
> Unfortunately, ParmVarDecl doesn't have quite enough information to do
> this. We currently point at the parameter name (or the place it would
> occur, for unnamed parameters). We might be able to cram some more
> source range information into DeclaratorChunk::ParamInfo and look back
> at that to get the underlining...

If we have the Declarator object laying around, I think that would be  
a nice extension.  Declarator itself isn't very good at preserving  
source range information (unfortunately).  It even has a  
Declarator::getSourceRange() method that returns an empty range right  
now.

I don't think it's a big priority, but this would be a very nice thing  
to add to Declarator and DeclaratorChunk::ParamInfo at some point.  If  
you're interested in this, please tackle it.  If not, don't worry  
about it :)

Thanks a lot for making the changes Doug,

-Chris