[cfe-commits] [patch] Qualifiers refactor

[Douglas Gregor]

On Sep 23, 2009, at 2:04 AM, John McCall wrote:

> The attached patch refactors how qualifiers are represented in clang.
>
> Currently, QualType uses the bottom three bits of a type pointer to  
> represent the const, volatile, and restrict qualifiers.  Extended  
> qualifiers are represented using an ExtQualType object, which is an  
> ordinary node in the type hierarchy --- except that it isn't.  Most  
> common operations on types have to explicitly check for these  
> special qualifier nodes and handle them differently.

... except that most common operations end up forgetting that  
ExtQualType exists. One of the big benefits of this change is that all  
of the qualifiers are kept together, so it's easier to reason about  
them as a whole. Plus, ExtQualType didn't fit well into the type  
system; this does.

> This patch changes QualType to store only two qualifiers directly on  
> the pointer;  the third bit is repurposed to signal whether the  
> pointer is a Type or an ExtQuals object (which holds, but is not, a  
> Type).  This makes it very cheap to decide whether a type carries  
> any qualifiers, as well as to compute the unqualified version of a  
> type.
>
> Because there are only two remaining bits, it's necessary to store  
> 'volatile' on the ExtQuals object, 'const' being ubiquitous and  
> 'restrict' being common in certain system headers.

Right. A quick check of my system headers found 53507 uses of 'const',  
1691 uses of 'restrict', and 609 uses of 'volatile'.

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

A few minor comments:

In lib/CodeGen/CGValue.h,

@@ -187,21 +173,18 @@ public:
    bool isPropertyRef() const { return LVType == PropertyRef; }
    bool isKVCRef() const { return LVType == KVCRef; }

-  bool isVolatileQualified() const { return Volatile; }
-  bool isRestrictQualified() const { return Restrict; }
-  unsigned getQualifiers() const {
-    return (Volatile ? QualType::Volatile : 0) |
-           (Restrict ? QualType::Restrict : 0);
-  }
+  bool isVolatileQualified() const { return Quals.hasVolatile(); }
+  bool isRestrictQualified() const { return Quals.hasRestrict(); }
+  unsigned getVRQualifiers() const { return Quals.getCVRQualifiers(); }

getVRQualifiers() should mask off the Const bit.

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

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

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.

In lib/Sema/SemaOverload.cpp, BuiltinCandidateTypeSet should probably  
have a blanket FIXME to cope with extended qualification. I don't  
quite know what the semantics should be, but we'll have to think about  
it at some point.

@@ -340,12 +340,14 @@ QualType Sema::ConvertDeclSpecToType(con
      if (Result->isFunctionType() && TypeQuals) {
        // Get some location to point at, either the C or V location.
        SourceLocation Loc;
-      if (TypeQuals & QualType::Const)
+      if (TypeQuals & DeclSpec::TQ_const)
          Loc = DS.getConstSpecLoc();

@@ -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 ;)

Very nice!

	- Doug

Before:

clang-cc -token-cache /tmp/tokencache INPUTS/Cocoa_h.m
name	  avg  	  min  	  med  	  max  	   SD  	 total
user	 0.2142	 0.2140	 0.2142	 0.2144	 0.0002	 1.0710
  sys	 0.0319	 0.0320	 0.0326	 0.0314	 0.0005	 0.1597
wall	 0.2504	 0.2502	 0.2512	 0.2501	 0.0005	 1.2520

After:

name	  avg  	  min  	  med  	  max  	   SD  	 total
user	 0.2234	 0.2234	 0.2234	 0.2236	 0.0001	 1.1172
  sys	 0.0320	 0.0333	 0.0309	 0.0312	 0.0009	 0.1598
wall	 0.2596	 0.2609	 0.2584	 0.2590	 0.0009	 1.2982