[PATCH] D21453: Add support for attribute "overallocated"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: include/clang/AST/Decl.h:3250
+  /// This is true if this struct ends with an array marked 'flexible_array'.
+  bool HasFlexibleArrayAttr : 1;
+
----------------
rsmith wrote:
> How is this different from `HasFlexibleArrayMember`? Do we really need both?
As I commented below, we don't need both if we are going to treat an array with flexible_array attribute like other existing flexible arrays.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2173
+def err_flexible_array_nested : Error<
+  "struct with a member marked 'flexible_array' cannot be nested">;
 def err_aligned_attribute_argument_not_int : Error<
----------------
rsmith wrote:
> I don't think it's very clear what "nested" means here. I assume you mean that a struct with a `flexible_array` member can't be used as the type of a field. If so, why not? C allows that for other kinds of flexible array members.
Yes, that's what I meant. I disallowed using struct with a flexible_array member to be used as a field in another struct just because the use case I had in mind didn't need that (see the link below). We don't need to restrict its usage if we are going to handle it like other existing flexible array members.

http://lists.llvm.org/pipermail/cfe-dev/2016-February/047561.html


================
Comment at: lib/AST/ExprConstant.cpp:5200
+    } else
       Result.Designator.setInvalid();
     return true;
----------------
rsmith wrote:
> Should we also set `HasFlexibleArrayAttr` on *real* flexible array members (which will be fields with `IncompleteArrayType`) rather than invalidating the designator?
I'm assuming you are also suggesting we call Result.addArray on "real" flexible arrays too?


https://reviews.llvm.org/D21453