[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

In https://reviews.llvm.org/D32210#730553, @jroelofs wrote:

> This doesn't forbid assigning them to block properties... is that intentional?
>
>   typedef void (^Block)(int);
>  
>   @interface Foo
>   @property Block B;
>   @end
>  
>   void foo(Foo *f, Block __attribute__((noescape)) b) {
>     f.B = b;
>   }
>


No, it's not intentional. The code to check assignments of noescape blocks probably had to go to ActOnBinOp or BuildBinOp to forbid this too.



================
Comment at: include/clang/Basic/AttrDocs.td:122
+* Cannot be returned from a function
+* Cannot be captured by a block
+* Cannot be assigned to a variable
----------------
TheRealAdamKemp wrote:
> This may be too restrictive in some cases. Consider this:
> 
> ```
> typedef void (^BlockTy)();
> void nonescapingFunc(__attribute__((noescape)) BlockTy);
> 
> void callerFunc(__attribute__((noescape)) BlockTy block) {
>   nonescapingFunc(^{ block(); });
> }
> ```
> 
> It sounds like the above code would give an error, but the capturing block is also nonescaping, which means it should be allowed. This is a useful pattern, and disallowing it would make these attributes very cumbersome.
> 
> The code could also be written like this:
> 
> ```
> typedef void (^BlockTy)();
> void nonescapingFunc(__attribute__((noescape)) BlockTy);
> 
> void callerFunc(__attribute__((noescape)) BlockTy block) {
>   BlockTy wrapBlock = ^{
>     block();
>   };
>   nonescapingFunc(wrapBlock);
> }
> ```
> 
> Again, I would expect that to not give an error or at least be able to decorate the declaration of `wrapBlock` to indicate that it is also nonescaping (can this attribute be applied to locals or only function arguments?).
I didn't think forbidding capturing noescape blocks would be too restrictive in practice, but I see your point. If it's a common pattern, we probably shouldn't forbid that.


================
Comment at: lib/Sema/SemaChecking.cpp:2567
+        else
+          assert(FDecl->isVariadic() &&
+                 "Called function expected to be variadic");
----------------
aaron.ballman wrote:
> What about functions that have no prototype (they're not variadic, but they accept any number of arguments)? Should include a test for this.
You are right, this assertion should check for functions without prototypes too.


https://reviews.llvm.org/D32210