[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 9:04 PM, Doug Gregor wrote:
> On Tue, Oct 21, 2008 at 8:33 PM, Chris Lattner <clattner at apple.com>  
> wrote:
>> 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).
>>> +  /// 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.
>
> We're now using isAtLeastAsQualifiedAs in
> Sema::CheckPointerTypesForAssignment. That currently ignores
> address-space qualifiers, but adding those asserts would turn code
> that would now compile into something that crashes the compiler.

I agree that it is a risk, however, code in that situation is probably  
just being silently miscompiled. If we get through 'make test' without  
regressions, I'd be happy to be aggressive on this.  Clang is at a  
point in its development where we can afford to expose breakage  
aggressively.

>>> +  /// 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.
>
> I'm going to fix all of these at once in a later batch.

Ok!

>>> @@ -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?
>
> I don't think it's necessary unless we need this kind of check again  
> later.

ok, maybe a static function?  When it was one "isa" I was fine with it  
being inlined.  As it grows, I'm less comfortable with it.

Some other thoughts about it:

ImplicitParamDecl isa VarDecl, so I think you can drop the  
ImplicitParamDecl (I know that this isn't new to this patch).

Does this change the behavior of C by adding FunctionDecl?

>>> +++ 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);
>
> Why not just make GetNonReferenceType inline? It's small enough, and
> it's going to happy in lots of hot code paths.

Sure, as long as it doesn't require #including new headers in a .h file.

-Chris