This is an archive of the discontinued LLVM Phabricator instance.

[attributes] Add an attribute "os_consumes_this" with similar semantics to "ns_consumes_self"
ClosedPublic

Authored by george.karpenkov on Nov 30 2018, 5:18 PM.

Diff Detail

Event Timeline

I know that the error message is not quite right, but this has nothing to do with this patch.
I'll see on which level it could be fixed: currently the error for mismatch for CXXMethodDecl still prints "function".

george.karpenkov retitled this revision from [attributes] Add another attribute, os_consumes_this, with similar semantics to ns_consumes_self to [attributes] Add an attribute "os_consumes_this" with similar semantics to "ns_consumes_self".
george.karpenkov edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Dec 1 2018, 10:09 AM
clang/include/clang/Basic/AttrDocs.td
887–891

Similarly -> Similar

clang/test/Sema/attr-osobject.cpp
51

Missing some more test coverage:

  • Attribute takes no args
  • Attribute does not appertain to a static member function

Question: what should happen in this code?

struct S {
  virtual void frobble() [[clang::os_consumes_this]];
};

struct T : S {
  void frobble() override;
};

int main() {
  S *s = new T;
  T *t = static_cast<T *>(s);

  s->frobble();
  t->frobble();
}

Does the attribute get inherited by the overrider?

(This question really applies to the other attributes in this area as well, but it cropped up while I was thinking about this pointers.)

george.karpenkov marked an inline comment as done.Dec 5 2018, 10:41 AM

Does the attribute get inherited by the overrider?

Yes conceptually: https://reviews.llvm.org/D55154, we propagate it in the analyzer.
No in the AST: I don't think it makes sense to propagate it during AST construction

aaron.ballman added inline comments.Dec 6 2018, 7:07 AM
clang/include/clang/Basic/Attr.td
851

Given how trivial the implementation is in SemaDeclAttr.cpp, I think a better approach would be to add a new SubsetSubject similar to NonStaticNonConstCXXMethod and use that as the subject list here. Then you can remove the new custom diagnostic and handler as well. Also, shouldn't this be limited to member functions, not non-member functions?

852

What do you think about splitting this out from the retain docs into consume docs (same for the two attributes below)? Or is there enough overlap between the two sections that you'd just wind up duplicating a lot of content?

clang/include/clang/Basic/AttrDocs.td
889

I'd drop the second e.g., as it reads strangely. Also, it should be e.g., for the first one.

clang/test/Sema/attr-osobject.cpp
8

What this pointer is being consumed here? I would have expected this to have a diagnostic.

51

But... this is a function?

Also, spurious semi-colon after the function body.

george.karpenkov marked 7 inline comments as done.Dec 6 2018, 1:28 PM
george.karpenkov added inline comments.
clang/include/clang/Basic/Attr.td
851

I'm a bit confused: aren't CXXMethods already member functions by definition?

852

Yeah I think they should be grouped together.

clang/test/Sema/attr-osobject.cpp
8

What do you mean? This function does not have a body, but a valid implementation could be e.g. this->release()

george.karpenkov updated this revision to Diff 177037.
george.karpenkov marked an inline comment as done.

Looks close, just a few minor testing concerns.

clang/include/clang/Basic/Attr.td
851

They are, but they're not necessarily always instance methods. For instance, they can be static member functions without a this pointer. I suppose I used confusing terminology though by talking about member vs nonmember, sorry about that.

852

As you've seen, that causes some duplication in the resulting documentation. I suspect we may want to refactor the docs at some point, but that shouldn't hold up this patch.

clang/test/Sema/attr-osobject.cpp
8

That's my fault -- my eyes glazed over that this was inside the structure declaration -- I thought the }; was a line earlier for some reason. :-P

Can you add a second test that uses a static member function, which should be diagnosed.

51

Still has the spurious semicolon.

george.karpenkov marked 2 inline comments as done.

done

This revision is now accepted and ready to land.Dec 6 2018, 1:58 PM
This revision was automatically updated to reflect the committed changes.