rsmith added inline comments.
================
Comment at: include/clang/AST/Expr.h:631-634
@@ -630,1 +630,6 @@
+ /// \brief If the current Expr is either a pointer, this will try to
+ /// statically determine the number of bytes available where the pointer is
+ /// pointing. Returns true if all of the above holds and we were able to
+ /// figure out the size, false otherwise.
+ ///
----------------
Looks OK, but you have an undesirable "either" in the comment now.
================
Comment at: include/clang/Basic/AttrDocs.td:272
@@ +271,3 @@
+ using ``__asm__("foo")`` to explicitly name your functions, thus preserving
+ your ABI; also, non-overloadable C functions with pass_object_size are
+ not mangled.
----------------
Missing `` around pass_object_size here.
================
Comment at: include/clang/Basic/AttrDocs.td:317
@@ +316,3 @@
+candidates are otherwise equally good, then the overload with one or more
+parameters with pass_object_size is preferred. This implies that the choice
+between two identical overloads both with ``pass_object_size`` on one or more
----------------
And here.
================
Comment at: include/clang/Basic/AttrDocs.td:351-355
@@ +350,7 @@
+
+* Only one use of pass_object_size is allowed per parameter.
+
+* It is an error to take the address of a function with pass_object_size on any
+ of its parameters. If you wish to do this, you can create an overload without
+ pass_object_size on any parameters.
+
----------------
And here.
================
Comment at: include/clang/Basic/AttrDocs.td:359
@@ +358,3 @@
+ are not pointers. Additionally, any parameter that ``pass_object_size`` is
+ applied to must be marked const at its function's definition.
+ }];
----------------
And around "const" here, maybe.
================
Comment at: include/clang/Sema/Initialization.h:832
@@ +831,3 @@
+ /// \brief Trying to take the address of a function that doesn't support
+ /// having its address taken
+ FK_AddressOfUnaddressableFunction,
----------------
Add a period at the end of this comment.
================
Comment at: lib/AST/ExprConstant.cpp:6507-6509
@@ -6506,5 +6545,3 @@
// handle all cases where the expression has side-effects.
- // Likewise, if Type is 3, we must handle this because CodeGen cannot give
a
- // conservatively correct answer in that case.
- if (E->getArg(0)->HasSideEffects(Info.Ctx) || Type == 3)
return Success((Type & 2) ? 0 : -1, E);
----------------
I don't disagree, but it seems logically similar to the `HasSideEffects` case,
which is still here. Maybe that should be moved to `CodeGen` too.
================
Comment at: lib/AST/ExprConstant.cpp:9534
@@ +9533,3 @@
+
+ Result = llvm::APInt(/*NumBits=*/64, Size, /*IsSigned=*/true);
+ return true;
----------------
I would expect this to produce a `size_t`, rather than an arbitrary-seeming
signed 64-bit integer. It also seems suspicious to be converting a uint64_t to
a signed `APInt` here.
================
Comment at: lib/AST/ItaniumMangle.cpp:2205
@@ +2204,3 @@
+
+ if (FD != nullptr) {
+ if (auto *Attr = FD->getParamDecl(I)->getAttr<PassObjectSizeAttr>()) {
----------------
`if (FD)` is more common in these parts.
================
Comment at: lib/CodeGen/CGCall.cpp:110
@@ +109,3 @@
+ // pass_object_size. So, we preallocate for the common case.
+ prefix.reserve(FPT->getNumParams());
+
----------------
Given that this appends, `reserve(prefix.size() + FPT->getNumParams())` seems
better.
================
Comment at: lib/CodeGen/CGCall.cpp:2847-2849
@@ -2803,4 +2846,5 @@
EmitCallArg(Args, *Arg, ArgTypes[I]);
+ MaybeEmitImplicitObjectSize(I, *Arg);
EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
CalleeDecl, ParamsToSkip + I);
}
----------------
These seem to be in the wrong order, and the call to `Args.back()` will
misbehave if a parameter is marked with `__attribute__((nonnull,
pass_object_size(N)))`. Swap these calls over.
================
Comment at: lib/CodeGen/CGCall.cpp:2862-2864
@@ -2817,4 +2861,5 @@
EmitCallArg(Args, *Arg, ArgTypes[I]);
+ MaybeEmitImplicitObjectSize(I, *Arg);
EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
CalleeDecl, ParamsToSkip + I);
}
----------------
Likewise.
================
Comment at: lib/Sema/SemaOverload.cpp:8832-8834
@@ +8831,5 @@
+
+ auto HasPassObjSize = std::mem_fn(&ParmVarDecl::hasAttr<PassObjectSizeAttr>);
+
+ auto I = std::find_if(FD->param_begin(), FD->param_end(), HasPassObjSize);
+ if (I == FD->param_end()) {
----------------
Any reason why you factor out `HasPassObjSize` here and not in the previous
(essentially identical) function? And can you factor this out into a
`getPassObjectSizeParamIndex` function or something, to avoid some of the
duplication between this and the previous function?
http://reviews.llvm.org/D13263
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
