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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
