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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Decl.h:4272-4275
+    FieldDecl *FD = nullptr;
+    for (FieldDecl *Field : fields())
+      FD = Field;
+    return FD;
----------------
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.


================
Comment at: clang/include/clang/AST/Decl.h:4277-4282
+  const FieldDecl *getLastField() const {
+    const FieldDecl *FD = nullptr;
+    for (const FieldDecl *Field : fields())
+      FD = Field;
+    return FD;
+  }
----------------
void wrote:
> aaron.ballman wrote:
> > We use this pattern fairly often to avoid repeating the implementation.
> Done. I just hate the use of `const_cast`...
Yeah, I usually go take a shower after that sort of thing. :-D


================
Comment at: clang/include/clang/Basic/Attr.td:4246
+    private:
+      SourceRange countedByFieldLoc;
+    public:
----------------
void wrote:
> erichkeane wrote:
> > 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?
> I tried that before and it didn't work. The issue is that at the time of parsing we don't yet have the full definition of the structure / union. So the `Decl`'s aren't really available to us yet. There may be a way to massage the parsing code to allow this to happen, but I'd like to do it as a separate patch. I'll document it with a `FIXME`.
I think at some point we're going to want this to take an `Expr` argument instead of an identifier argument, so we can support `foo.bar` and `baz[0]`, etc as arguments for more complicated situations. But I believe the current form is reasonable as-is (we can start taking an expression and all of these identifiers should become a `MemberExpr`, so users don't have to change their code).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:7200
+  let Content = [{
+Clang supports the ``counted_by`` attribute for the flexible array member of a
+structure. The argument for the attribute is the name of a field member in the
----------------
void wrote:
> aaron.ballman wrote:
> > If we land the changes with the attribute as a C-only attribute, we should document that explicitly here (unless we're immediately enabling it C++ mode; not asking for busywork).
> That's not something normally done for other `COnly` attributes. (However, the documentation shows those attributes being available for C++11, which is probably a mistake...) I went ahead and added that this is for C though.
Heh, our attribute documentation has been slowly getting better over time but is still pretty inconsistent; thank you for adding it!


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6368-6369
 
+def warn_flexible_array_counted_by_attr_field_not_found : Warning<
+  "counted_by field %0 not found">;
+def err_flexible_array_counted_by_attr_refers_to_self : Error<
----------------
void wrote:
> aaron.ballman wrote:
> > Why is this a warning and not a typical `err_no_member` error? I think we want that behavior so that we get typo correction for free. e.g.,
> > ```
> > struct S {
> >   int Count;
> >   int FAM[] __attribute__((counted_by(Clount))); // Should get a "did you mean 'Count'" fix-it
> > };
> > ```
> I thought it would be too harsh to make this an error. But I suppose there's no harm in doing this. I'm not sure how to implement a fixit for a closely matching identifier. Do you have an example of how to find the closest matching Decl?
I'll leave a comment below with what I'm thinking.


================
Comment at: clang/lib/AST/DeclBase.cpp:443-444
+
+  if (const auto *OID = dyn_cast_if_present<ObjCIvarDecl>(D))
+    return OID->getNextIvar() == nullptr;
+
----------------
void wrote:
> aaron.ballman wrote:
> > Can you explain a bit about what this code is for? I didn't spot any test coverage for it, but perhaps I missed something.
> Short answer is I don't know. :-) This is copied from the original implementation of this method.
Good enough for me, thank you. :-) (I made that comment before I realized this was a refactor, I forgot to come back and delete it.)


================
Comment at: clang/lib/Sema/SemaDecl.cpp:18029
+
+    S.LookupName(Result, Scope);
+    if (Result.getResultKind() == LookupResult::Found) {
----------------
When the lookup result is not found, I think you should call `Sema::DiagnoseEmptyLookup()`. This will handle giving you the error about the member not being found, but it should also give you typo correction for free, and if typo correction happens, you should then already know what new declaration was found via the correction, so you can continue to check for the other cases from there.

https://github.com/llvm/llvm-project/blob/b57df9fe9a1a230f277d671bfa0884bbda9fc1c5/clang/lib/Sema/SemaLambda.cpp#L1151 is an example of this being called elsewhere.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:18035
+        << ECA->getCountedByField() << SR;
+  } else if (!Field->getType()->isIntegerType()) {
+    // The "counted_by" field must have an integer type.
----------------
void wrote:
> aaron.ballman wrote:
> > Errr any integer type?
> > ```
> > struct S {
> >   bool HerpADerp;
> >   int FAM[] __attribute__((counted_by(HerpADerp)));
> > };
> > ```
> > seems like something we don't want, but I also wonder about enumeration types and more interestingly, plain `char` where it could be treated as signed or unsigned depending on compiler flags.
> Yeah...unless they want just 1 element. ;-) I can rule a `bool` out. (Done)
> 
> A `char` should be fine. Dunno about signed vs. unsigned...
The `char` situation I'm thinking about is morally: https://godbolt.org/z/377WPYojz

With `-fsigned-char`, accessing `Idx` means you get the value `-1` and with `-funsigned-char`, you get the value `255`. If that was used to index into `FAM`, the `signed char` version would become a bounds access issue whereas the `unsigned char` version works fine, so it's hard to write a portable attribute that refers to a `char` member.

Then again, the `counted_by` attribute is intended to catch exactly this kind of issue, so perhaps it's fine on `char`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381