aaron.ballman added a comment.
> The attribute will first be used by the Swift compiler in a new implicit
> bridging diagnostic, but may have other non-Swift use-cases for diagnostics.
This means there's no use of this attribute within Clang which suggests to me
that this attribute belongs in the Swift compiler until there's a reason to
upstream it. Or are you planning on using this attribute within Clang in the
Very Near Future?
================
Comment at: clang/include/clang/Basic/Attr.td:1957
+}
+
def AssumeAligned : InheritableAttr {
----------------
You should define these attributes as being mutually exclusive; e.g.,
```
def : MutualExclusions<[Escape, NoEscape]>;
```
================
Comment at: clang/include/clang/Basic/AttrDocs.td:260
+``escape`` placed on a function parameter of a pointer type is used to indicate
+that the pointer can escape the function. This means that a reference to the
object
+the pointer points to that is derived from the parameter value may survive
----------------
guitard0g wrote:
> NoQ wrote:
> > Are you sure you want "can escape" instead of "will escape"? In the former
> > case it's impossible to implement the warning without false positives
> > ("well, it says it may escape, but I know for sure that if I pass these
> > other arguments then it won't escape"). In the latter case, of course, a
> > lot of places where the value escapes conditionally won't be able to wear
> > the attribute. Do I understand correctly that you want to proceed with the
> > more aggressive diagnostic?
> Hmm that's an interesting question. Initially my thought was that any
> possibility of escaping should eliminate the option of passing in a temporary
> pointer, which I think is reasonable in Swift's implicit bridging use-case.
> In terms of the API contract, I was thinking that any API author who
> annotates their function parameter with 'escaping' is effectively saying you
> shouldn't pass in a pointer that you're not comfortable escaping. I think it
> may be a little restrictive to say that the pointer will always escape for
> certain, but I do think that anybody using a function that takes an escaping
> parameter should treat this as if it were always escaping (if that makes
> sense). I suppose my question then would be, do you think it's better to say
> "will escape" if that's what the client calling into an API should expect
> (even though the pointer might not always escape in the implementation)?
> ``escape`` placed on a function parameter of a pointer type is used to
> indicate that the pointer can escape the function.
FWIW, this reads a bit strangely to me because that's already the case without
the attribute. Anything that's not marked *can* escape. I think "will escape"
more clearly identifies to the person reading the function signature that the
parameter should be treated as though it will escape. If it does not escape,
that's not necessarily an issue because that shouldn't change how the user
treats the argument (for example, it may not escape today but it may start to
escape in the future).
One thing the docs are missing is an explanation of what benefit you get from
using this annotation. Does this improve optimizations? Enable better checking
somewhere? That sort of thing (with examples) would be good to add.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1568
- D->addAttr(::new (S.Context) NoEscapeAttr(S.Context, AL));
+ switch (AL.getKind()) {
+ default:
----------------
This should be handled directly from Attr.td, I'll put some comments in there.
================
Comment at: clang/test/SemaObjCXX/escape.mm:1-2
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -std=c++1z %s
+
----------------
I don't see any use of blocks, so I don't think you need `-fblocks`. I think
you can also remove the C++11 RUN line as it doesn't really add any test
coverage.
================
Comment at: clang/test/SemaObjCXX/escape.mm:24
+// expected-note {{conflicting attribute is here}}
+void invalidFunc5(__attribute__((noescape)) __attribute__((escape)) int *); //
expected-error {{'escape' and 'noescape' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
----------------
Can you also add test coverage for the more likely case of inheriting mutually
exclusive attributes? e.g.,
```
void foo() __attribute__((escape));
void foo() __attribute__((noescape));
void bar() __attribute__((noescape));
void bar() __attribute__((escape));
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107026/new/
https://reviews.llvm.org/D107026
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
