[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

[Douglas Gregor]

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.

> +  /// 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().

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

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

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

   if (Candidate)

> +          QualType TConvType = Context.getCanonicalType(TopConv- 
> >getType());
> +          if (ConvType == TConvType) {
> +            Candidate = false;
> +            break;
> +          }
> +        }
> +      }

This is an O(n^2) algorithm :(

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()
   };

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
   };

   - Doug