george.burgess.iv 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.
+ ///
----------------
rsmith wrote:
> Looks OK, but you have an undesirable "either" in the comment now.
Danke
================
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);
----------------
rsmith wrote:
> 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.
They seem sufficiently different to me. GCC's docs for `__builtin_object_size`
say "__builtin_object_size never evaluates its arguments for side-effects. If
there are any side-effects in them, it returns (size_t) -1 for type 0 or 1 and
(size_t) 0 for type 2 or 3," so this is more a feature of the builtin itself
than an artifact of how we generate code. That said, I can't tell if our
partial relaxation of this restriction is intentional or not. :)
FWIW, the static analyzer also apparently depends on us returning `Success`
here if there are side-effects; tests fail if I move this check to CodeGen.
================
Comment at: lib/CodeGen/CGCall.cpp:110
@@ +109,3 @@
+ // pass_object_size. So, we preallocate for the common case.
+ prefix.reserve(FPT->getNumParams());
+
----------------
rsmith wrote:
> Given that this appends, `reserve(prefix.size() + FPT->getNumParams())` seems
> better.
Nice catch -- thanks.
================
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()) {
----------------
rsmith wrote:
> 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?
> Any reason why you factor out HasPassObjSize here and not in the previous
> (essentially identical) function?
Artifact of not cleaning up completely after playing with different ways to do
this -- sorry.
> And can you factor this out into a getPassObjectSizeParamIndex function or
> something, to avoid some of the duplication between this and the previous
> function?
I tried merging the functions entirely; if we don't like the result, I'm happy
to go back to the way they were and just factor out the `find_if` bit. :)
http://reviews.llvm.org/D13263
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
