[cfe-dev] Adding access to array size expressions in ConstantArrayType.

[Eli Friedman]

On Tue, Jun 30, 2009 at 12:35 PM, Enea Zaffanella<zaffanella at cs.unipr.it> wrote:
> @@ -816,6 +816,11 @@
>   /// 'int X[static restrict 4]'. For function parameters only.
>   unsigned IndexTypeQuals : 3;
>
> +  /// LBLoc - The location of the left bracket.
> +  SourceLocation LBLoc;
> +  /// RBLoc - The location of the right bracket.
> +  SourceLocation RBLoc;
> +

These members appear to be unused.

> Index: include/clang/Parse/DeclSpec.h
> ===================================================================
> --- include/clang/Parse/DeclSpec.h      (revision 74387)
> +++ include/clang/Parse/DeclSpec.h      (working copy)
> @@ -488,11 +488,16 @@
>     /// True if this dimension was [*].  In this case, NumElts is null.
>     bool isStar : 1;
>
> +    /// The location of the left and right brackets.
> +    SourceLocation* RBracketLoc;
> +
>     /// This is the size of the array, or null if [] or [*] was specified.
>     /// Since the parser is multi-purpose, and we don't want to impose a
> root
>     /// expression class on all clients, NumElts is untyped.
>     ActionBase::ExprTy *NumElts;
> -    void destroy() {}
> +    void destroy() {
> +      delete RBracketLoc;
> +    }
>   };
>
>   /// ParamInfo - An array of paraminfo objects is allocated whenever a
> function
> @@ -696,13 +701,14 @@
>   ///
>   static DeclaratorChunk getArray(unsigned TypeQuals, bool isStatic,
>                                   bool isStar, void *NumElts,
> -                                  SourceLocation Loc) {
> +                                  SourceLocation LBLoc, SourceLocation
> RBLoc) {
>     DeclaratorChunk I;
>     I.Kind          = Array;
> -    I.Loc           = Loc;
> +    I.Loc           = LBLoc;
>     I.Arr.TypeQuals = TypeQuals;
>     I.Arr.hasStatic = isStatic;
>     I.Arr.isStar    = isStar;
> +    I.Arr.RBracketLoc = new SourceLocation(RBLoc);
>     I.Arr.NumElts   = NumElts;
>     return I;
>   }

Don't allocate a single SourceLocation on the heap; SourceLocations
are really small (less than or equal to the size of a pointer), so
this doesn't actually save any space.  Also, I'd just name the member
EndLoc; having it around could also be useful for function
declarators, and perhaps a couple of other kinds of declarators.

> Index: lib/Sema/SemaTemplateInstantiate.cpp
> ===================================================================
> --- lib/Sema/SemaTemplateInstantiate.cpp        (revision 74387)
> +++ lib/Sema/SemaTemplateInstantiate.cpp        (working copy)
> @@ -382,9 +382,23 @@
>   IntegerLiteral ArraySize(Size, SizeType, Loc);
>   return SemaRef.BuildArrayType(ElementType, T->getSizeModifier(),
>                                 &ArraySize, T->getIndexTypeQualifier(),
> -                                Loc, Entity);
> +                                SourceRange(), Entity);
>  }

It would be nice to have a proper source range here; can you at least
put in a FIXME?

> +QualType
> +TemplateTypeInstantiator::InstantiateConstantArrayWithExprType
> +(const ConstantArrayWithExprType *T) const {
> +  const ConstantArrayType *CAT = T;
> +  return InstantiateConstantArrayType(CAT);
> +}
> +
> +QualType
> +TemplateTypeInstantiator::InstantiateConstantArrayWithoutExprType
> +(const ConstantArrayWithoutExprType *T) const {
> +  const ConstantArrayType *CAT = T;
> +  return InstantiateConstantArrayType(CAT);
> +}

There's no point to the extra variables; just pass in T directly to
InstantiateConstantArrayType.

>  QualType
>  TemplateTypeInstantiator::
>  InstantiateIncompleteArrayType(const IncompleteArrayType *T) const {
> @@ -393,8 +407,8 @@
>     return ElementType;
>
>   return SemaRef.BuildArrayType(ElementType, T->getSizeModifier(),
> -                                0, T->getIndexTypeQualifier(),
> -                                Loc, Entity);
> +                                0, T->getIndexTypeQualifier(),
> +                                SourceRange(),  Entity);
>  }

Please add a FIXME for the missing source range.

>  QualType
> @@ -429,7 +443,8 @@
>
>   return SemaRef.BuildArrayType(ElementType, T->getSizeModifier(),
>                                 InstantiatedArraySize.takeAs<Expr>(),
> -                                T->getIndexTypeQualifier(), Loc, Entity);
> +                                T->getIndexTypeQualifier(),
> +                                SourceRange(), Entity);
>  }

Please add a FIXME for the missing source range.

> Index: lib/AST/ASTContext.cpp
> ===================================================================
> --- lib/AST/ASTContext.cpp      (revision 74387)
> +++ lib/AST/ASTContext.cpp      (working copy)
> @@ -274,6 +274,10 @@
>     Align = getTypeAlign(cast<ArrayType>(T)->getElementType());
>     break;
>
> +  case Type::ConstantArrayWithExpr:
> +  case Type::ConstantArrayWithoutExpr:
> +    T = T->getCanonicalTypeInternal().getTypePtr();
> +    // Intentionally falling through next case.

Why do you need to call getCanonicalTypeInternal here?  Why can't they
just share the case with ConstantArray?

> @@ -1184,7 +1245,8 @@
>
>  QualType ASTContext::getIncompleteArrayType(QualType EltTy,
>                                             ArrayType::ArraySizeModifier
> ASM,
> -                                            unsigned EltTypeQuals) {
> +                                            unsigned EltTypeQuals,
> +                                            SourceRange Brackets) {
>   llvm::FoldingSetNodeID ID;
>   IncompleteArrayType::Profile(ID, EltTy, ASM, EltTypeQuals);
>
> @@ -1199,7 +1261,7 @@
>
>   if (!EltTy->isCanonical()) {
>     Canonical = getIncompleteArrayType(getCanonicalType(EltTy),
> -                                       ASM, EltTypeQuals);
> +                                       ASM, EltTypeQuals, Brackets);
>
>     // Get the new insert position for the node we care about.
>     IncompleteArrayType *NewIP =
> @@ -1207,8 +1269,9 @@
>     assert(NewIP == 0 && "Shouldn't be in the map!"); NewIP = NewIP;
>   }
>
> -  IncompleteArrayType *New = new (*this,8) IncompleteArrayType(EltTy,
> Canonical,
> -                                                           ASM,
> EltTypeQuals);
> +  IncompleteArrayType *New
> +    = new (*this,8) IncompleteArrayType(EltTy, Canonical,
> +                                        ASM, EltTypeQuals, Brackets);
>
>   IncompleteArrayTypes.InsertNode(New, InsertPos);
>   Types.push_back(New);

Unlike the other array types you're adding Brackets to,
IncompleteArrayType gets canonicalized; we don't want a source range
on a canonical IncompleteArrayType, and we don't want to end up with
the wrong SourceRange on a non-canonical IncompleteArrayType.  It's a
little tricky, so if the issues I'm talking about aren't obvious, I'd
suggest skipping adding the bracket locations to IncompleteArrayType
for now.

Otherwise, this is looking good!

-Eli