aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+ let Spellings = [CXX11<"gsl", "Owner">];
+ let Subjects = SubjectList<[CXXRecord]>;
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > Has Microsoft already added support for this? We had an unfortunate
> > > > problem with `gsl::suppress` where we introduced the attribute first
> > > > and MSVC introduced it under a different, incompatible syntax. I would
> > > > strongly like to avoid repeating that.
> > > I will double check this. Her Sutter's paper have this spelling and I
> > > believe that the Microsoft implementation is following the same spelling.
> > > The main difference is that the MSVC version does not have a type
> > > argument at this point but they do plan to add it as an optional
> > > argument. (They want to infer the pointee type when it is not provided.)
> > > The clang community do not want the inference, hence we made the type
> > > argument required. Always providing the type argument should be
> > > compatible with future versions of the MSVC implementation.
> > I take this to mean that we're not implementing the same attribute as MSVC
> > is, and that worries me.
> >
> > > Always providing the type argument should be compatible with future
> > > versions of the MSVC implementation.
> >
> > The problem is that code written for MSVC (where the type argument is not
> > required) will not compile with Clang (where the type argument is
> > required), if I understand properly.
> >
> > Generally speaking, when we implement an attribute from another vendor
> > namespace, we expect the attribute to have the same semantics as whoever
> > "owns" that vendor namespace. It's a bit trickier here because `gsl` isn't
> > really a vendor so much as a collective, so I don't know who is the
> > authority to turn to.
> >
> > On a completely different note: would this attribute also make sense within
> > C with a C2x spelling?
> I see, that is a valid concern. A followup patch makes the type argument
> optional so it will be fully compatible with MSVC. I don't think it makes
> sense with C or at least I am not sure what would I annotate using these
> attributes in C code as the subjects are classes. I know that it is possible
> to simulate classes in C but I am not sure how well the analysis would work
> in such cases.
>
> Are you OK with making the argument optional in a followup patch or do you
> want us to cherry-pick that modification into this one?
> I don't think it makes sense with C or at least I am not sure what would I
> annotate using these attributes in C code as the subjects are classes. I know
> that it is possible to simulate classes in C but I am not sure how well the
> analysis would work in such cases.
I couldn't think of a reason to expose this in C either, so I think the current
spelling is good as-is.
> Are you OK with making the argument optional in a followup patch or do you
> want us to cherry-pick that modification into this one?
I'd prefer the argument be optional in this patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63954/new/
https://reviews.llvm.org/D63954
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
