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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I've got no further comments/concerns.



================
Comment at: clang/include/clang/AST/Decl.h:4272-4275
+    FieldDecl *FD = nullptr;
+    for (FieldDecl *Field : fields())
+      FD = Field;
+    return FD;
----------------
aaron.ballman wrote:
> void wrote:
> > aaron.ballman wrote:
> > > Could this be implemented as: `return !field_empty() ? *std::prev(field_end()) : nullptr;` ? Then maybe someday we'll get better iterator support that isn't forward-only and this code will automagically get faster.
> > Using `std::prev` on a forward iterator won't work:
> > 
> > https://stackoverflow.com/questions/23868378/why-stdprev-does-not-fire-an-error-with-an-iterator-of-stdunordered-set
> > 
> > `std::prev` itself is defined only for bidirectional iterators:
> > 
> > ```
> >  template<typename _BidirectionalIterator>
> >     _GLIBCXX_NODISCARD
> >     inline _GLIBCXX17_CONSTEXPR _BidirectionalIterator
> >     prev(_BidirectionalIterator __x, typename
> >          iterator_traits<_BidirectionalIterator>::difference_type __n = 1)
> >     {
> > ...
> > ```
> > 
> Drat! Oh well, it was a lovely thought.
something like `!field_empty() ? *std::advance(field_begin(), std::distance(field_begin(), field_end() - 1) : nullptr`

would do what Aaron would like, though perhaps with an extra run through the fields list until then.


================
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<
----------------
void wrote:
> erichkeane wrote:
> > 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 ...` ?
> I reworded them a bit.
> 
> As for referring to itself, it could be a bit confusing to say that it 'cannot point to itself' because the 'itself' in that is the attribute, not the flexible array member. I think it's more-or-less clear what the error's referring to. The user can use this attribute only with a flexible array member. So they should know what what's meant by the message.
I think the reword helps, I am less confident that referring to the 'flexible array member' is clear. One of the things I've noticed in the past is users 'trying' attributes without looking them up to see what they do.  Frankly, I think that should be a supported strategy, and one that we manage by making clear diagnostics.

I'm up for suggestions/hopeful someone else will come up with something better, but I can hold my nose at this if we don't have anything better (and in fact, my attempts to put a strawman up were at least as bad).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148381/new/

https://reviews.llvm.org/D148381