🍒[Clang] Permit noescape on non-pointer types #10735
Conversation
In modern C++ we often use span, string_view or other view objects instead of raw pointers. This PR opens the door to mark those noescape. This can be useful to document the API contracts, for interop with memory safe languages like Swift or Rust, and possibly in the future to implement take this into account in codegen. The PR also adds a feature test macro. The goal is to help avoiding warnings when the code is compiler by earlier versions of clang that does not permit this attribute on non-pointer types. rdar://140196481
|
@swift-ci please test LLVM |
| @@ -3,5 +3,11 @@ | |||
| template<typename T> | |||
| void test1(T __attribute__((noescape)) arr, int size); | |||
|
|
|||
| // expected-warning@+1 {{'noescape' attribute only applies to pointer arguments}} | |||
| void test2(int __attribute__((noescape)) arr, int size); | |||
There was a problem hiding this comment.
I'm guessing this is also the case in the original patch, but this diagnostic looks off to me. 0 is invalid doesn't really mean anything. We should fix this.
CC @Xazax-hun
There was a problem hiding this comment.
I agree that we could improve the phrasing. Let's wait until @Xazax-hun can work on it – this PR is just making the rebranch CI happy.
There was a problem hiding this comment.
Oh, I just re-read your comment again, and I now agree we should fix the 0 being printed instead of the actual type, let me put up a PR.
| S.Diag(AL.getLoc(), diag::warn_attribute_pointers_only) | ||
| if (!isValidPointerAttrType(T, /* RefOkay */ true) && !T->isRecordType()) { | ||
| S.Diag(AL.getLoc(), | ||
| diag::warn_attribute_pointer_or_reference_or_record_only) | ||
| << AL << AL.getRange() << 0; |
There was a problem hiding this comment.
This is where the 0 comes from in the diagnostic. The previous diagnostic looks like this:
def warn_attribute_pointers_only : Warning<
"%0 attribute only applies to%select{| constant}1 pointer arguments">,
InGroup<IgnoredAttributes>;
So we likely want to do something like << T instead of << 0.
| @@ -281,4 +281,30 @@ StringLiteral *FormatMatchesAttr::getFormatString() const { | |||
| return cast<StringLiteral>(getExpectedFormat()); | |||
| } | |||
|
|
|||
| bool clang::isValidPointerAttrType(QualType T, bool RefOkay) { | |||
There was a problem hiding this comment.
Should some of the changes like this also go to upstream clang as well?
There was a problem hiding this comment.
Yeap, I think so! Gabor already started the upstreaming work in llvm#117344.
Original PR: #9789
rdar://151918708