[cfe-commits] [patch] Qualifiers refactor

[John McCall]

Douglas Gregor wrote:
> On Sep 23, 2009, at 2:04 AM, John McCall wrote:
>>  This could be avoided by using 16-byte alignment on Types, which 
>> would be extremely straightforward to do under the patch (modulo any 
>> possible required changes to the allocator, which I haven't 
>> investigated).
>>
>> This patch also audits many of the uses of qualifiers in clang, 
>> adjusting them to handle arbitrary qualifiers (or be augmentable for 
>> future qualifiers) when that was reasonably straightforward.
>
> I think this patch is great. The one downside I see is that it causes 
> Clang -fparse-only on Cocoa.h (from a token cache) to run about 4% 
> slower (#'s at the end). However, I'm willing to pay that cost to get 
> the API improvements this patch brings.

Presumably that's with the errant debugging statements removed. :)

That's actually many more volatiles in the system headers than I was 
expecting.  It really might be worthwhile to grab that extra bit if 
we're going to see so many volatiles.

> A few minor comments:
>
> @@ -1675,8 +1674,10 @@ QualType Sema::CXXCheckConditionalOperan
>            // The type 'Q Pointee (MoreDerived::*)' is the common type.
>            // We don't use ImpCastExprToType here because this could 
> still fail
>            // for ambiguous or inaccessible conversions.
> -          QualType Common = Context.getMemberPointerType(
> -            LPointee.getQualifiedType(Q), MoreDerived.getTypePtr());
> +          LPointee
> +            = Context.getQualifiedType(LPointee, 
> Qualifiers::fromCVRMask(CVR));
> +          QualType Common
> +            = Context.getMemberPointerType(LPointee, 
> MoreDerived.getTypePtr());
>            if (PerformImplicitConversion(LHS, Common, "converting"))
>              return QualType();
>            if (PerformImplicitConversion(RHS, Common, "converting"))
>
> I think this will lose non-CVR qualifiers on the pointer type (the 
> previous code probably had the same issue). Of course, it doesn't look 
> like we're checking that the pointers have the same address spaces, 
> either.

Fixed.  Or, well, it checks address spaces now;  it doesn't handle 
address-space promotion.

> @@ -1778,16 +1779,21 @@ QualType Sema::FindCompositePointerType(
>           I = QualifierUnion.begin(),
>           E = QualifierUnion.end();
>         I != E; (void)++I, ++MOC) {
> +    Qualifiers Quals = Qualifiers::fromCVRMask(*I);
>      if (MOC->first && MOC->second) {
>        // Rebuild member pointer type
> -      Composite1 = 
> Context.getMemberPointerType(Composite1.getQualifiedType(*I),
> -                                                MOC->first);
> -      Composite2 = 
> Context.getMemberPointerType(Composite2.getQualifiedType(*I),
> -                                                MOC->second);
> +      Composite1 = Context.getMemberPointerType(
> +                                    
> Context.getQualifiedType(Composite1, Quals),
> +                                    MOC->first);
> +      Composite2 = Context.getMemberPointerType(
> +                                    
> Context.getQualifiedType(Composite2, Quals),
> +                                    MOC->second);
>      } else {
>        // Rebuild pointer type
> -      Composite1 = 
> Context.getPointerType(Composite1.getQualifiedType(*I));
> -      Composite2 = 
> Context.getPointerType(Composite2.getQualifiedType(*I));
> +      Composite1
> +        = Context.getPointerType(Context.getQualifiedType(Composite1, 
> Quals));
> +      Composite2
> +        = Context.getPointerType(Context.getQualifiedType(Composite2, 
> Quals));
>      }
>    }
>
> Same concern here; we're probably losing non-CVR qualifiers.

Also fixed.

> Looking at the template argument deduction code, I thought of an 
> amusing case that we don't need to support (but might be a little 
> harder now):
>
>     template<typename T>
>     struct address_space_of {
>         static const unsigned value = 0;
>     };
>
>     template<typename T, unsigned N>
>     struct address_space_of<T * __attribute__((address_space(N))> {
>         static const unsigned value = N;
>     };
>
> Don't worry about this case now, of course.

Yeah, I've actually been considering that possibility;  I have no idea 
how to resolve it.  Dependent attributes would be an entirely new thing, 
I think.

It'd probably be extremely useful to people doing C++ embedded work, 
although that might be a vanishingly small set of people.

> @@ -2415,18 +2385,21 @@ QualType ASTContext::getArrayDecayedType
>
>    QualType PtrTy = getPointerType(PrettyArrayType->getElementType());
>
> +  llvm::errs() << "array type decaying from " << Ty.getAsString() << 
> " to " << PtrTy.getAsString() << "\n";
> +  llvm::errs() << "element type is " << 
> PrettyArrayType->getElementType().getAsString() << "\n";
> +
>
> You might want to remove those debugging statements ;)

Whoops. :)

John.