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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1222
+def NoEscape : InheritableAttr {
+  let Spellings = [GCC<"noescape">, CXX11<"clang", "noescape">];
+  let Subjects = SubjectList<[ParmVar], WarnDiag, "ExpectedParameter">;
----------------
I do not think GCC supports this attribute, so that should be using a GNU spelling rather than a GCC spelling.


================
Comment at: include/clang/Basic/Attr.td:1223
+  let Spellings = [GCC<"noescape">, CXX11<"clang", "noescape">];
+  let Subjects = SubjectList<[ParmVar], WarnDiag, "ExpectedParameter">;
+  let Documentation = [NoEscapeDocs];
----------------
No need to specify the last two arguments, they should be handled by default already.


================
Comment at: include/clang/Basic/AttrDocs.td:118
+  let Content = [{
+``noescape`` placed on a block parameter is used to inform the compiler that the block passed to a function cannot escape: that is, the block will not be invoked after the function returns. To ensure that the block does not escape, clang imposes the following restrictions on usage of non-escaping blocks:
+
----------------
Please wrap to 80 columns.

"block parameter" is a bit strange -- I assumed that meant parameter to a block, not a function parameter of block type. May want to clarify.

Should also clarify "the block will not be invoked after the function returns." Does this code produce undefined behavior?
```
  typedef void (^BlockTy)();
  void nonescapingFunc(__attribute__((noescape)) BlockTy);
  void escapingFunc(BlockTy);

  void callerFunc(__attribute__((noescape)) BlockTy block) {
    nonescapingFunc(block); // OK
    escapingFunc(block);    // error: parameter is not annotated with noescape
  }

  void f(void) {
    BlockTy Blk = ^{};
    callerFunc(Blk);
    Blk();
  }
```


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3094
+def err_noescape_passed_to_call : Error<
+  "cannot pass non-escaping parameter '%0' to function or method expecting escaping "
+  "argument">;
----------------
expecting an escaping argument

Also, someday we should make non-foo vs nonfoo a consistent decision in our diagnostics. We seem to use both forms.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3106
+def note_noescape_parameter : Note<
+  "parameter is %select{annotated|not annotated}0 with noescape">;
+
----------------
Should quote `noescape`.


================
Comment at: lib/Sema/SemaChecking.cpp:2545
+                                   SourceLocation CallSiteLoc) {
+  ArrayRef<ParmVarDecl*> Params = FDecl->parameters();
+
----------------
Space before the asterisk.


================
Comment at: lib/Sema/SemaChecking.cpp:2548
+  for (unsigned I = 0, E = Args.size(); I < E ; ++I) {
+    const auto *CalleePD = I < Params.size() ? Params[I] : nullptr;
+
----------------
Please don't use `auto` here.


================
Comment at: lib/Sema/SemaChecking.cpp:2559
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(Args[I]->IgnoreImpCasts())) {
+      const auto *CallerPD = DRE->getDecl();
+      if (CallerPD->hasAttr<NoEscapeAttr>()) {
----------------
Don't use `auto` here.


================
Comment at: lib/Sema/SemaChecking.cpp:2562
+        S.Diag(Args[I]->getExprLoc(), diag::err_noescape_passed_to_call)
+            << CallerPD->getName();
+        S.Diag(CallerPD->getLocation(), diag::note_noescape_parameter) << 0;
----------------
No need to use `getName()` here, you can just pass in the declaration directly. That means you should also remove the quotes from the diagnostic so it does not get double-quoted.


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


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1520-1522
+  ParmVarDecl *PD = dyn_cast<ParmVarDecl>(D);
+  if (!PD)
+    return;
----------------
Is there ever a case where this if statement will early return? I think you can just use `cast<ParmVarDecl>(D)->getType()` below.


================
Comment at: lib/Sema/SemaExpr.cpp:11180
+      if (const auto *DRE =
+              dyn_cast_or_null<DeclRefExpr>(RHS.get()->IgnoreImpCasts())) {
+        const auto *D = DRE->getDecl();
----------------
Why `dyn_cast_or_null`? I didn't think `IgnoreImpCasts()` could return null.


================
Comment at: lib/Sema/SemaExpr.cpp:11181
+              dyn_cast_or_null<DeclRefExpr>(RHS.get()->IgnoreImpCasts())) {
+        const auto *D = DRE->getDecl();
+        if (D->hasAttr<NoEscapeAttr>()) {
----------------
Don't use `auto`.


================
Comment at: lib/Sema/SemaExpr.cpp:11184
+          Diag(DRE->getLocation(), diag::err_noescape_assignment)
+              << D->getName();
+          Diag(D->getLocation(), diag::note_noescape_parameter) << 0;
----------------
Can drop the `getName()` call.


================
Comment at: lib/Sema/SemaExpr.cpp:13682
+  if (Var->hasAttr<NoEscapeAttr>()) {
+    S.Diag(Loc, diag::err_noescape_block_capture) << Var->getName();
+    S.Diag(Var->getLocation(), diag::note_noescape_parameter) << 0;
----------------
Can drop the `getName()` call.


================
Comment at: lib/Sema/SemaStmt.cpp:3203
+  if (const auto *DRE = dyn_cast_or_null<DeclRefExpr>(RetValExp)) {
+    const auto *D = DRE->getDecl();
+    if (D->hasAttr<NoEscapeAttr>()) {
----------------
Don't use `auto`.


================
Comment at: lib/Sema/SemaStmt.cpp:3205
+    if (D->hasAttr<NoEscapeAttr>()) {
+      Diag(DRE->getLocation(), diag::err_noescape_returned) << D->getName();
+      Diag(D->getLocation(), diag::note_noescape_parameter) << 0;
----------------
Can drop `getName()`.


https://reviews.llvm.org/D32210