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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/AST/ItaniumMangle.cpp:2700
+      if (FD->getParamDecl(I)->hasAttr<NoEscapeAttr>())
+        Out << "U8noescape";
     }
----------------
rjmccall wrote:
> 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.
Oh that's right. We should avoid breaking ABI when retroactively adding noescape.


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


================
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";
----------------
rjmccall wrote:
> I know this is existing code, but you should probably at least warn about ns_consumed mismatches here in non-ARC.
I made warn_nsconsumed_attribute_mismatch and warn_nsreturns_retained_attribute_mismatch warnings that get promoted to errors when ARC is enabled.


================
Comment at: lib/Sema/SemaExpr.cpp:7180
+  if (!FromType->hasExtParameterInfos())
+    return FromType;
+
----------------
rjmccall wrote:
> 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.
Yes. that's correct. The original implementation of the function removed noescape attributes from FromType's parameters to match ToType, but only if doing so made it a legal conversion.

I made changes to have this function take and return QualType and return QualType() when noescape on parameters makes the conversion invalid.


================
Comment at: lib/Sema/SemaExpr.cpp:7251
+    if (const auto *rhproto = dyn_cast<FunctionProtoType>(rhptee))
+      rhptee = S.removeNoEscapeFromFunctionProto(lhproto, rhproto);
+
----------------
rjmccall wrote:
> I think the right place to do this is probably mergeFunctionTypes.
When we have the following conditional operator expression, what is the type of the expression?

```
void func0(__attribute__((noescape)) int *a, int *b);
void func1(int *a, __attribute__((noescape)) int *b);

c ? func0 : func1;
```

The standard says the type of each parameter in the composite parameter type list is the composite type of the corresponding parameters of both functions, so I guess mergeFunctionType should drop noescape from both parameters?


================
Comment at: lib/Sema/SemaTemplate.cpp:5477
+      }
+    }
+
----------------
rjmccall wrote:
> 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.
I removed this code so that noescape is handled the same way noreturn is handled, which causes clang to complain about "S4<&noescapeFunc2> e1;" in test/SemaObjCXX/noescape.mm when it is not compiling for c++17 or later.

Is that what you meant?


https://reviews.llvm.org/D32210