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 
  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 

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 

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.


cfe-commits mailing list

Reply via email to