void added inline comments.
================ Comment at: clang/include/clang/AST/Decl.h:4272-4275 + FieldDecl *FD = nullptr; + for (FieldDecl *Field : fields()) + FD = Field; + return FD; ---------------- erichkeane wrote: > 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. Yeah, I want to avoid unnecessary iterations through the list. ================ 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< ---------------- erichkeane wrote: > 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). I added a diagnostic (the first one) that indicates it must apply to a FAM. So maybe it'll be okay? I'm open to other wording... 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
