erichkeane added inline comments.
================ Comment at: clang/include/clang/AST/DeclBase.h:482 + /// Whether it resembles a flexible array member. + static bool isFlexibleArrayMemberLike( + ASTContext &Context, const Decl *D, QualType Ty, ---------------- Why isn't this a member function? ================ Comment at: clang/include/clang/Basic/Attr.td:4246 + private: + SourceRange countedByFieldLoc; + public: ---------------- aaron.ballman wrote: > Teeny tiniest of coding style nits :-D Should we instead be capturing the field itself, rather than its location? It seems to me that would be more useful? ================ Comment at: clang/include/clang/Basic/AttrDocs.td:7203 +same structure holding the count of elements in the flexible array. This +information can be used to improve the results of array bound sanitizer and the +``__builtin_dynamic_object_size``. ---------------- As this is the purpose of this attribute, it seems to me that the documentation should headline/more obviously highlight that the purpose here is to improve the result of the sanitizer when it comes to flexible array members. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6371 +def err_flexible_array_counted_by_attr_refers_to_self : Error< + "counted_by field %0 cannot refer to the flexible array">; +def err_flexible_array_counted_by_attr_field_not_integral : Error< ---------------- aaron.ballman wrote: > We try to wrap syntax elements in single quotes in our diagnostics so it's > more visually distinct This one isn't clear... something more specifically about 'cannot point to itself' is perhaps more useful/explainatory. Also, the beginning of all of these is really quite awkward as well, perhaps something like: `field %0 referenced by 'counted_by' attribute ...` ? ================ Comment at: clang/lib/AST/DeclBase.cpp:482 + + RecordDecl::field_iterator FI( + DeclContext::decl_iterator(const_cast<FieldDecl *>(FD))); ---------------- Whats going on here? Is this simply an attempt to see if this is the last one? Can we combine this effort with the 'getLastFieldDecl' above? Alternatively, can we get a good comment here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148381/new/ https://reviews.llvm.org/D148381 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
