[cfe-commits] r67265 - in /cfe/trunk: include/clang/AST/ include/clang/Parse/ lib/AST/ lib/CodeGen/ lib/Parse/ lib/Sema/ test/SemaCXX/ test/SemaTemplate/

[Douglas Gregor]

On Mar 18, 2009, at 10:42 PM, Chris Lattner wrote:

> On Mar 18, 2009, at 5:18 PM, Douglas Gregor wrote:
>
>> Author: dgregor
>> Date: Wed Mar 18 19:18:19 2009
>> New Revision: 67265
>> +class NestedNameSpecifier {
>> +  /// \brief Construct a nested name specifier that refers to a  
>> type.
>> +  NestedNameSpecifier(const Type *T) {
>> +    Data = reinterpret_cast<uintptr_t>(T);
>> +    assert((Data & 0x01) == 0 && "cv-qualified type in nested-name- 
>> specifier");
>> +    Data |= 0x01;
>> +  }
>
> Would it be reasonable to use PointerIntPair for this?  It would be  
> nice to only have the bitmangling code and assertions in one place.

Sure.

>  Alternatively, maybe having a DiscriminatedPointer<foo*, bar*>  
> class (which itself wraps PointerIntPair) would be better (perhaps  
> overkill).

Ted was agitating for something like this, too. It might be a fun  
hack, but in this case it wouldn't bring much of a benefit.

>
>
>> +/// \brief Represents a type that was referred to via a qualified
>> +/// name, e.g., N::M::type.
>> +///
>> +/// This type is used to keep track of a type name as written in the
>> +/// source code, including any nested-name-specifiers.
>
> It might be worth explicitly mentioning here that this a sugar type  
> like typedeftype.

Okay.

>
>>
>> +class QualifiedNameType : public Type, public llvm::FoldingSetNode {
>> +  /// \brief The number of components in the qualified name, not
>> +  /// counting the final type.
>
> How about adding:
> "In the example above, this would be 2 for N::M" ?

Will do.

>
>> +++ cfe/trunk/include/clang/Parse/DeclSpec.h Wed Mar 18 19:18:19 2009
>> @@ -397,10 +397,36 @@
>> /// specifier.
>> class CXXScopeSpec {
>>  SourceRange Range;
>> -  Action::CXXScopeTy *ScopeRep;
>> +
>> +  /// Storage containing the scope representations for up to four
>> +  /// levels of nested-name-specifier. NumScopeReps specifiers how
>> +  /// many levels there are. If there are more than four, we use
>> +  /// ManyScopeReps.
>> +  Action::CXXScopeTy *InlineScopeReps[4];
>> +
>> +  /// The number of scope representations we've stored.
>> +  unsigned NumScopeReps;
>> +
>> +  /// The number of scope representations we can store without
>> +  /// allocating new memory.
>> +  unsigned Capacity;
>
> Can this just use SmallVector<Action::CXXScopeTy *,4> ?  Why  
> reinvent it?

Heh, I tried that, and I have the scars to prove it. The problem is  
that DeclaratorChunk's representation of member pointers embeds a  
CXXScopeSpec using an aligned character array. When the  
DeclaratorChunk is returned from DeclaratorChunk::getMemberPointer, it  
copies the bits of the CXXScopeSpec rather than invoking the copy  
constructor [*]. SmallVector doesn't like to have its bits copied, but  
the InlineScopeReps representation is designed to allow such copying.  
I'm not thrilled about it, but I'm less thrilled about rewriting  
DeclaratorChunk to eliminate the union entirely :)

[*] Why not just invoke the copy constructor? Well... we're playing  
games with unions in DeclaratorChunk, because we're shoving non-POD  
types within the unions and pretending we didn't. We play similar  
games with function parameters, where we really wanted a SmallVector  
but couldn't have it because of the union.

>
>>
>
>> +++ cfe/trunk/lib/AST/Type.cpp Wed Mar 18 19:18:19 2009
>> @@ -96,6 +96,8 @@
>>  if (const ClassTemplateSpecializationType *Spec
>>        = dyn_cast<ClassTemplateSpecializationType>(this))
>>    return Spec->getCanonicalTypeInternal().getDesugaredType();
>> +  if (const QualifiedNameType *QualName  =  
>> dyn_cast<QualifiedNameType>(this))
>> +    return QualName->getNamedType().getDesugaredType();
>
> nit, only one space before the =.

Okay.

>
> Overall, this is an amazingly great improvement.  I'm hacking on  
> features.html, after it goes in, when convenient, can you please add  
> a specific example from this? Maybe a simplified std::string or  
> std::vector<t> example?

Sure thing.

	- Doug