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:
> > 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?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:335-341
+ if (attributeIsTypeArgAttr(*AttrName)) {
+ ParseAttributeWithTypeArg(*AttrName, AttrNameLoc, Attrs, EndLoc, ScopeName,
+ ScopeLoc, Syntax);
+ // FIXME: when attributeIsTypeArgAttr() is true, assumes that the attribute
+ // takes a single parameter.
+ return 1;
+ }
----------------
I don't like how this short-circuits the remainder of the logic in this
function. I'd rather see this one-off logic handled in
`ParseClangAttributeArgs()` if we have to treat it as a one-off. A better
approach would be to implement type arguments within
`ParseAttributeArgsCommon()`.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:2618-2619
+ } else if (isa<OwnerAttr>(NewAttribute) || isa<PointerAttr>(NewAttribute))
{
+ // gsl::Owner and gsl::Pointer are allowed to be added to a class after
it
+ // is defined.
+ ++I;
----------------
I'd like to see some tests for this.
I'm a bit worried about whether this will do good things in practice or not.
I'm concerned about situations like:
```
struct S;
void func(S *s) {
// Interesting things happen here.
}
struct [[gsl::Pointer]] S {};
```
and whether the "interesting things" will be properly [un]diagnosed.
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
