aaron.ballman added a comment.
In D63954#1562928 <https://reviews.llvm.org/D63954#1562928>, @erik.pilkington
wrote:
> > We explicitly allow to add an annotation after
> > the definition of the class to allow adding annotations
> > to external source from by the user, e.g.
> >
> > #include <vector>
> >
> > namespace std {
> > template<typename T, typename Alloc>
> > class [[gsl::Owner(T)]] vector;
> > }
>
> Wait, does that even work? What if the vector was declared in an inline
> namespace in the header? Seems like a strange recommendation for users. Is
> there some reason you can't just add these annotations to standard libraries?
> I doubt libcxx would have a problem with getting better warnings on their
> types.
We do not want to encourage users to redeclare things within namespace std like
this -- I believe that is UB.
================
Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+ let Spellings = [CXX11<"gsl", "Owner">];
+ let Subjects = SubjectList<[CXXRecord]>;
----------------
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.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:4167
+pointer/reference to the data owned by ``O``. The owned data is assumed to end
+its lifetime once the owning object's lifetime end.
+
----------------
the owning object's lifetime end -> the owning object's lifetime ends
================
Comment at: clang/include/clang/Basic/AttrDocs.td:4177
+ let Content = [{
+When annotating a class ``P`` with ``[[gsl::Pointer(T)]]``, it assumed to be a
+non-owning type whose objects can point to ``T`` type objects or dangle.
----------------
You don't reference `P` in the rest of the docs, is that intentional?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:4186
+}
\ No newline at end of file
----------------
Please keep the newline at the end of the file.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4537
+static void handleLifetimeCategoryAttr(Sema &S, Decl *D, const ParsedAttr &AL)
{
+ if (!AL.hasParsedType()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;
----------------
Under what circumstances is this check needed? I believe this is already
automatically handled for you.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+ if (ParmType->isVoidType()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" <<
AL;
+ return;
----------------
Is `void` really the only problematic type? What about incomplete types, for
instance?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4553
+ // we always add (and check) the attribute to the cannonical decl.
+ D = D->getCanonicalDecl();
+ if(AL.getKind() == ParsedAttr::AT_Owner) {
----------------
Will this work? What happens if we see a forward declaration with this
attribute first and then see the canonical declaration later? I suspect this
work needs to be done when merging declaration attributes instead.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4554
+ D = D->getCanonicalDecl();
+ if(AL.getKind() == ParsedAttr::AT_Owner) {
+ if (checkAttrMutualExclusion<PointerAttr>(S, D, AL))
----------------
Formatting.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4557
+ return;
+ if (const auto *Attr = D->getAttr<OwnerAttr>()) {
+ if (Attr->getDerefType().getTypePtr() != ParmType.getTypePtr()) {
----------------
Please use a name other than `Attr` (as that's a type name).
================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:61
+class [[gsl::Owner(int)]] AddTheSameLater;
\ No newline at end of file
----------------
Please add a newline to the end of the file.
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
