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