[cfe-commits] r57935 - in /cfe/trunk: include/clang/AST/Type.h lib/AST/Expr.cpp lib/Sema/Sema.h lib/Sema/SemaExpr.cpp lib/Sema/SemaOverload.cpp lib/Sema/SemaType.cpp test/SemaCXX/decl-expr-ambiguity.cpp test/SemaCXX/qualification-conversion.cpp

[Chris Lattner]

On Oct 21, 2008, at 4:43 PM, Douglas Gregor wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=57935&view=rev
> Log:
> Initial step toward supporting qualification conversions (C++ 4.4).

Nice!  Some minor thoughts:

>  - When building DeclRefExprs, strip away the reference type, since
>    there are no expressions whose type is a reference. We'll need to
>    do this throughout Sema.

Aha, so that is how it is supposed to work :), that makes a lot more  
sense.

> +  /// isMoreQualifiedThan - Determine whether this type is more
> +  /// qualified than the Other type. For example, "const volatile  
> int"
> +  /// is more qualified than "const int", "volatile int", and
> +  /// "int". However, it is not more qualified than "const volatile
> +  /// int".
> +  bool isMoreQualifiedThan(QualType Other) const {
> +    unsigned MyQuals = this->getCVRQualifiers();
> +    unsigned OtherQuals = Other.getCVRQualifiers();
> +    return MyQuals != OtherQuals && (MyQuals | OtherQuals) ==  
> MyQuals;
> +  }

You probably don't care about Addr space qualifiers, and it is fine to  
defer the issue... but it would probably be best to assert that  
"Other.getAddressSpace() == 0" (and 'this') until we handle them.   
Likewise in isAtLeastAsQualifiedAs.

>
> +  /// FIXME: Expressions can't have reference type, so the following
> +  /// isn't needed.
>   if (TR->isReferenceType()) // C++ [expr]
>     return LV_Valid;

I think a lot of the reference logic in the compiler is currently  
busted.  The only real case we support references today is for some  
vaarg builtins IIRC.  Does anything in the regression suite fail if  
you remove this today?  If not, I'd just rip it (and any other  
bogosity) out.

>
>
> @@ -356,7 +360,11 @@
>     return LV_Valid;
>   case DeclRefExprClass: { // C99 6.5.1p2
>     const Decl *RefdDecl = cast<DeclRefExpr>(this)->getDecl();
> -    if (isa<VarDecl>(RefdDecl) || isa<ImplicitParamDecl>(RefdDecl))
> +    if (isa<VarDecl>(RefdDecl) ||
> +        isa<ImplicitParamDecl>(RefdDecl) ||
> +        // C++ 3.10p2: An lvalue refers to an object or function.
> +        isa<FunctionDecl>(RefdDecl) ||
> +        isa<OverloadedFunctionDecl>(RefdDecl))

Does this make sense to be a predicate on valuedecl?

> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Tue Oct 21 18:43:52 2008
> @@ -450,7 +450,7 @@
>   }
>   // If this reference is not in a block or if the referenced  
> variable is
>   // within the block, create a normal DeclRefExpr.
> -  return new DeclRefExpr(VD, VD->getType(), Loc);
> +  return new DeclRefExpr(VD, GetNonReferenceType(VD->getType()),  
> Loc);

Very minor and pedantic, but this is a hot code path for both C and C+ 
+, and this adds a new out-of-line call.  How about something like:

QualType ValTy = VD->getType();
if (ValTy->isReferenceType()) ValTy = GetNonReferenceType(ValTy);
return new DeclRefExpr(VD, ValTy, Loc);

> +/// GetNonReferenceType - If Type is a reference type (e.g., const
> +/// int&), returns the type that the reference refers to ("const
> +/// int"). Otherwise, returns the type itself. This routine is used
> +/// throughout to implement C++ 5p6:
> +///
> +///   If an expression initially has the type "reference to  
> T" (8.3.2,
> +///   8.5.3), the type is adjusted to "T" prior to any further
> +///   analysis, the expression designates the object or function
> +///   denoted by the reference, and the expression is an lvalue.
> +QualType Sema::GetNonReferenceType(QualType Type)
> +{
> +  if (const ReferenceType *RefType = Type->getAsReferenceType())
> +    return RefType->getPointeeType();
> +  else
> +    return Type;
> +}

This seems more natural to be a method on QualType, what do you think?

Thanks Doug,

-Chris