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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/AttrDocs.td:138
+Additionally, for block pointers, the same restriction apply to copies of
+blocks. For example:
+
----------------
```
  Additionally, when the parameter is a `block pointer
  <https://clang.llvm.org/docs/BlockLanguageSpec.html>`, the same restriction applies
  to copies of the block.  For example:
```


================
Comment at: include/clang/Sema/Sema.h:1480
+  /// void (*to2)(__attribute__((noescape)) int *) = &from2;
+  ///
+  const FunctionProtoType *
----------------
I assume this only returns noescape from parameters that are not noescape in ToType.


================
Comment at: lib/AST/ItaniumMangle.cpp:2700
+      if (FD->getParamDecl(I)->hasAttr<NoEscapeAttr>())
+        Out << "U8noescape";
     }
----------------
This is not the right place to do this:

* It needs to be mangled in function types, not just on function declarations.  It looks like the code here supporting pass_object_size just gets this wrong.
* We don't want to mangle this in top-level declarations because doing so will make it more directly ABI-breaking to retroactively add this attribute to a declaration.

Instead, you should handle it in mangleExtParameterInfo.  Because these qualifiers are mangled in reverse alphabetical order, and "noescape" precedes "ns_consumed", it should be the last thing tested for in that function.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:14024
+
+  if (OldFT->hasExtParameterInfos())
+    for (unsigned I = 0, E = OldFT->getNumParams(); I != E; ++I)
----------------
I think we generally encourage the uses of braces when the nested statement is this complex, even if it is technically a single statement.


================
Comment at: lib/Sema/SemaDeclObjC.cpp:186
+            oldDecl->hasAttr<NSConsumedAttr>()) {
+      Diag(newDecl->getLocation(), diag::err_nsconsumed_attribute_mismatch);
+      Diag(oldDecl->getLocation(), diag::note_previous_decl) << "parameter";
----------------
I know this is existing code, but you should probably at least warn about ns_consumed mismatches here in non-ARC.


================
Comment at: lib/Sema/SemaExpr.cpp:7180
+  if (!FromType->hasExtParameterInfos())
+    return FromType;
+
----------------
So this function removes noescape attributes from FromType to match ToType, but only if it's a legal conversion?  Maybe it would be more reasonable to report an inability to convert in a more direct way, like by returning a null type.

Also, I think you're much more likely to avoid implicitly stripping sugar if you take both of these (or at least FromType?) as a QualType, and also return a QualType.  Or you could pass the original QualType down separately and just return that in the cases where you don't need to change anything.


================
Comment at: lib/Sema/SemaExpr.cpp:7251
+    if (const auto *rhproto = dyn_cast<FunctionProtoType>(rhptee))
+      rhptee = S.removeNoEscapeFromFunctionProto(lhproto, rhproto);
+
----------------
I think the right place to do this is probably mergeFunctionTypes.


================
Comment at: lib/Sema/SemaTemplate.cpp:5477
+      }
+    }
+
----------------
I think this is not the right place to be doing this; instead, we should be applying a conversion to the argument expression in CheckTemplateArgument.  It looks like we already do that in C++17 mode, because C++17 broadened the rules here to allow an arbitrary function conversion (among the other things allowed by the "converted constant expression" rules, [temp.arg.nontype]p2).  We probably could just leave it at that, since earlier standards are (in their infinite wisdom) stricter about type-matching here.  If that poses a serious compatibility problem, the right fix is to modify the clause in CheckTemplateArgument for non-type template parameters where it deals with pointers, references, and member pointers to functions to specifically allow this conversion.  But I'll note that we don't seem to be similarly generous about noreturn.


https://reviews.llvm.org/D32210