kees added a comment.
In D148381#4645600 <https://reviews.llvm.org/D148381#4645600>, @void wrote:
> Added more error messages. Changed some code around to align with coding
> practices. Added some more test cases.
Doing a full "allmodconfig" Linux kernel build with my 200ish __counted_by
annotations is building cleanly. Yay! :)
================
Comment at: clang/test/Sema/attr-counted-by.c:21
+ struct bar *fam[] __counted_by(non_integer); // expected-note {{counted_by
field 'non_integer' declared here}}
+};
----------------
void wrote:
> aaron.ballman wrote:
> > Please add test with the other strange FAM-like members, as well as one
> > that is just a trailing constant array of size 2.
> >
> > Some more tests or situations to consider:
> > ```
> > struct S {
> > struct {
> > int NotInThisStruct;
> > };
> > int FAM[] __counted_by(NotInThisStruct); // Should this work?
> > };
> >
> > int Global;
> > struct T {
> > int FAM[] __counted_by(Global); // Should fail per your design but is
> > that the behavior you want?
> > };
> >
> > struct U {
> > struct {
> > int Count;
> > } data;
> > int FAM[] __counted_by(data.Count); // Thoughts?
> > };
> >
> > struct V {
> > int Sizes[2];
> > int FAM[] __counted_by(Sizes[0]); // Thoughts?
> > };
> > ```
> > (I'm not suggesting any of this needs to be accepted in order to land the
> > patch.)
> >
> > You should also have tests showing the attribute is diagnosed when applied
> > to something other than a field, given something other than an identifier,
> > is given zero arguments, and is given two arguments.
> I added a "this isn't a flexible array member" error message.
>
> ```
> struct S {
> struct {
> int NotInThisStruct;
> };
> int FAM[] __counted_by(NotInThisStruct); // Should this work?
> };
> ```
>
> Yes, I believe it should. Kees had a similar example:
>
> ```
> struct S {
> int x;
> struct {
> int count;
> int FAM[] __counted_by(count);
> };
> };
> ```
>
> I made changes to support this. It may be controversial, so please let me
> know if you or anyone has an issue with it.
>
> ```
> int Global;
> struct T {
> int FAM[] __counted_by(Global); // Should fail per your design but is that
> the behavior you want?
> };
> ```
>
> Yes, it should fail. I'll add a test case.
>
> ```
> struct U {
> struct {
> int Count;
> } data;
> int FAM[] __counted_by(data.Count); // Thoughts?
> };
> ```
>
> I don't think we should support that at the moment. Referencing arbitrary
> fields in a nested structure is very complicated and the syntax for it is not
> at all settled on.
>
> ```
> struct V {
> int Sizes[2];
> int FAM[] __counted_by(Sizes[0]); // Thoughts?
> };
> ```
>
> Hmm...Not sure. I'll ask the GCC person who's implementing this on her side
> about this.
> Yes, I believe it should. Kees had a similar example:
>
> ```
> struct S {
> int x;
> struct {
> int count;
> int FAM[] __counted_by(count);
> };
> };
> ```
>
> I made changes to support this. It may be controversial, so please let me
> know if you or anyone has an issue with it.
FWIW, the Linux kernel has a LOT of this style (in an anonymous struct), which
is why I wanted to make sure it was supported in this first version of the
feature.
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
