rsmith 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;
+
----------------
How is this different from `HasFlexibleArrayMember`? Do we really need both?
================
Comment at: include/clang/Basic/AttrDocs.td:2078-2079
+Structs or classes that have an array member marked ``flexible_array`` cannot
be nested or subclassed.
+This attribute is useful when you want __builtin_object_size to be conservative
+when computing the size of an over-allocated array. For example:
+
----------------
Instead of "This attribute is useful when you want", I would suggest "This
attribute causes", since that is far from the only effect. It should also
disable the sanitizer checks for array bounds violations, for example.
================
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<
----------------
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.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2520
+ "variables, functions, methods, types, enumerations, enumerators, labels,
and non-static data members|"
+ "fields}1">,
InGroup<IgnoredAttributes>;
----------------
aaron.ballman wrote:
> non-static data members instead of fields.
Again, this will be confusing in C. Our diagnostics generally use the word
"field" to refer to fields in C or non-static data members in C++.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2170
+ "'flexible_array' attribute only applies to %select{"
+ "the last member of a struct|members of structs or classes|"
+ "fixed sized array members|array members that have at least one element}0">;
----------------
ahatanak wrote:
> aaron.ballman wrote:
> > I think "members of structs or classes" and "the last member of a struct"
> > could be combined into "the last member of a non-union class", instead of
> > using separate diagnostic text. What do you think?
> I agree, I think these two diagnostics should be combined.
We shouldn't talk about classes when targeting C.
================
Comment at: lib/AST/ExprConstant.cpp:287
Entries.back().ArrayIndex += N;
if (Entries.back().ArrayIndex > MostDerivedArraySize) {
diagnosePointerArithmetic(Info, E, Entries.back().ArrayIndex);
----------------
Pointer arithmetic that leaves the initial portion of a member with the
`flexible_array` attribute will lose its designator here. Is that what you want?
================
Comment at: lib/AST/ExprConstant.cpp:5200
+ } else
Result.Designator.setInvalid();
return true;
----------------
Should we also set `HasFlexibleArrayAttr` on *real* flexible array members
(which will be fields with `IncompleteArrayType`) rather than invalidating the
designator?
================
Comment at: lib/CodeGen/CGExpr.cpp:695-696
if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
- RecordDecl::field_iterator FI(
- DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
- return ++FI == FD->getParent()->field_end();
+ if (FD->getParent()->hasFlexibleArrayMemberOrAttr())
+ return true;
+ // For compatibility with existing code, we treat arrays of length 0 or
----------------
This is wrong. We shouldn't disable all bounds checking for all array members
in a class just because it ends in a flexible array member: only that member
should have its checks disabled. You can reverse the sense of this as a
fast-path for the common case of a class without a flexible array member,
though.
https://reviews.llvm.org/D21453
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
