[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Erich Keane via Phabricator via cfe-commits]

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