[cfe-commits] r81576 - in /cfe/trunk: include/clang/AST/DeclCXX.h lib/AST/DeclCXX.cpp lib/Sema/SemaExprCXX.cpp test/SemaCXX/conversion-delete-expr.cpp

[Fariborz Jahanian]

On Sep 11, 2009, at 3:37 PM, Douglas Gregor wrote:

>
> On Sep 11, 2009, at 2:44 PM, Fariborz Jahanian wrote:
>
>> Author: fjahanian
>> Date: Fri Sep 11 16:44:33 2009
>> New Revision: 81576
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=81576&view=rev
>> Log:
>> Patch to build visible conversion function list lazily and make its
>> first use in calling the conversion function on delete statements.
>
> Cool.
>
>> Added:
>>   cfe/trunk/test/SemaCXX/conversion-delete-expr.cpp
>> Modified:
>>   cfe/trunk/include/clang/AST/DeclCXX.h
>>   cfe/trunk/lib/AST/DeclCXX.cpp
>>   cfe/trunk/lib/Sema/SemaExprCXX.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=81576&r1=81575&r2=81576&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
>> +++ cfe/trunk/include/clang/AST/DeclCXX.h Fri Sep 11 16:44:33 2009
>> @@ -367,9 +367,15 @@
>>  /// Conversions - Overload set containing the conversion functions
>>  /// of this C++ class (but not its inherited conversion
>>  /// functions). Each of the entries in this overload set is a
>> -  /// CXXConversionDecl.
>> +  /// CXXConversionDecl.
>>  OverloadedFunctionDecl Conversions;
>>
>> +  /// VisibleConversions - Overload set containing the conversion  
>> functions
>> +  /// of this C++ class and all those inherited conversion  
>> functions that
>> +  /// are visible in this class. Each of the entries in this  
>> overload set is
>> +  /// a CXXConversionDecl or a FunctionTemplateDecl.
>> +  OverloadedFunctionDecl VisibleConversions;
>> +
>>  /// \brief The template or declaration that this declaration
>>  /// describes or was instantiated from, respectively.
>>  ///
>> @@ -570,6 +576,20 @@
>>    return &Conversions;
>>  }
>>
>> +  /// getVisibleConversionFunctions - get all conversion functions  
>> visible
>> +  /// in current class; including conversion function templates.
>> +  OverloadedFunctionDecl *getVisibleConversionFunctions(ASTContext  
>> &Context,
>> +                                                         
>> CXXRecordDecl *RD);
>
> The "RD" parameter is an implementation detail; it shouldn't leak  
> into the interface. Please provide a version of  
> getVisibleConversionFunctions that doesn't have this parameter, so  
> that clients don't have to worry about it.

Done.

>
>
>> +  /// addVisibleConversionFunction - Add a new conversion function  
>> to the
>> +  /// list of visible conversion functions.
>> +  void addVisibleConversionFunction(ASTContext &Context,
>> +                                    CXXConversionDecl *ConvDecl);
>> +
>> +  /// \brief Add a new conversion function template to the list of  
>> visible
>> +  /// conversion functions.
>> +  void addVisibleConversionFunction(ASTContext &Context,
>> +                                    FunctionTemplateDecl *ConvDecl);
>> +
>
> In all three of the new functions, you can remove the ASTContext  
> parameter. Instead, use Decl::getASTContext().
Done. There are more and I will clean them up shortly.

>
>
>> +/// getVisibleConversionFunctions - get all conversion functions  
>> visible
>> +/// in current class; including conversion function templates.
>> +OverloadedFunctionDecl *
>> +CXXRecordDecl::getVisibleConversionFunctions(ASTContext &Context,
>> +                                             CXXRecordDecl *RD) {
>> +  // If visible conversion list is already evaluated, return it.
>> +  if (RD == this &&
>> +      VisibleConversions.function_begin() !=  
>> VisibleConversions.function_end())
>> +    return &VisibleConversions;
>
> Checking whether VisibleConversions is empty isn't the same as  
> checking whether we've already tried to compute visible conversions.  
> With this formulation, we could spend a lot of time traversing a  
> class hierarchy that has no user-defined conversions anywhere in it.  
> Why not have a ComputedVisibleConversions bit in the CXXRecordDecl?

Good catch, oversight on my part. Fixed.

>
>
>> +  QualType ClassType = Context.getTypeDeclType(this);
>> +  if (const RecordType *Record = ClassType->getAs<RecordType>()) {
>> +    OverloadedFunctionDecl *Conversions
>> +      = cast<CXXRecordDecl>(Record->getDecl())- 
>> >getConversionFunctions();
>> +    for (OverloadedFunctionDecl::function_iterator
>> +         Func = Conversions->function_begin(),
>> +         FuncEnd = Conversions->function_end();
>> +         Func != FuncEnd; ++Func) {
>> +      if (FunctionTemplateDecl *ConversionTemplate =
>> +            dyn_cast<FunctionTemplateDecl>(*Func)) {
>> +        RD->addVisibleConversionFunction(Context,  
>> ConversionTemplate);
>> +        continue;
>> +      }
>
> Conversion templates can shadow other conversion templates, too, e.g.,
>
>  struct Base {
>    template<typename T> operator T*() const;
>  };
>
>  struct Derived : Base {
>    template<typename T> operator T*() const; // hides base conversion
>  };
>
> so we'll need to check that as well.
We now do. Added a test case for this.

>
>
>> +      CXXConversionDecl *Conv = cast<CXXConversionDecl>(*Func);
>> +      bool Candidate = true;
>> +      // Only those conversions not exact match of conversions in  
>> current
>> +      // class are candidateconversion routines.
>> +      if (RD != this) {
>> +        OverloadedFunctionDecl *TopConversions = RD- 
>> >getConversionFunctions();
>> +        QualType ConvType = Context.getCanonicalType(Conv- 
>> >getType());
>> +        for (OverloadedFunctionDecl::function_iterator
>> +             TFunc = TopConversions->function_begin(),
>> +             TFuncEnd = TopConversions->function_end();
>> +             TFunc != TFuncEnd; ++TFunc) {
>> +          CXXConversionDecl *TopConv =  
>> cast<CXXConversionDecl>(*TFunc);
>
> Can't *TFunc be a FunctionTemplateDecl? It looks like it from the  
> code below, under
Yes, fixed.

>
>
>  if (Candidate)
>
>> +          QualType TConvType = Context.getCanonicalType(TopConv- 
>> >getType());
>> +          if (ConvType == TConvType) {
>> +            Candidate = false;
>> +            break;
>> +          }
>> +        }
>> +      }
>
> This is an O(n^2) algorithm :(
Yes. I have added a FIXME. Will look at it.

>
>
> Also, you'll need to check cv-qualifiers on the functions. For  
> example, I don't think there's any hiding in this example:
>
>  struct Base {
>    operator int();
>  };
>
>  struct Derived : Base {
>    operator int() const; // not the same function as Base's non- 
> const operator int()
>  };
>

This just works because QualType for the two conversions don;t match.  
However, we have no overload resolution
to select one over the other in a delete statement. I have added a  
test case and FIXME in the test case for future work.

> Finally, there may be multiple bases with the same conversion. Both  
> conversion functions need to show up, e.g.,
>
>  struct Base1 {
>    operator int();
>  };
>
>  struct Base2 {
>    operator int();
>  };
>
>  struct Derived : Base1, Base2 {
>    // two operator int()s are visible here
>  };
>

Yes.

Fixed in TOT: http://llvm.org/viewvc/llvm-project?view=rev&revision=81618

- Fariborz

>  - Doug