xazax.hun added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+ let Spellings = [CXX11<"gsl", "Owner">];
+ let Subjects = SubjectList<[CXXRecord]>;
----------------
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?
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
