rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land.
Some minor typographical comments. Please add some tests for the union case, then this LGTM. ================ Comment at: lib/AST/ExprConstant.cpp:6333-6336 @@ +6332,6 @@ + if (BaseType->isArrayType()) { + // Because __builtin_object_size treats arrays as objects, we can ignore + // the index iff this is the last array in the Designator. + if (I + 1 == E) + return true; + auto *CAT = cast<ConstantArrayType>(Ctx.getAsArrayType(BaseType)); ---------------- OK, but please reflect that this is specific to `__builtin_object_size` in the name or at least doc comment for this function. ================ Comment at: lib/AST/ExprConstant.cpp:6361 @@ +6360,3 @@ + +/// Tests to see if the has a designator (that isn't necessarily valid). +static bool hasDesignator(const LValue &LVal) { ---------------- This is missing a noun. ================ Comment at: lib/AST/ExprConstant.cpp:6362 @@ +6361,3 @@ +/// Tests to see if the has a designator (that isn't necessarily valid). +static bool hasDesignator(const LValue &LVal) { + if (LVal.Designator.Invalid || !LVal.Designator.Entries.empty()) ---------------- I think this would be clearer if it were named `hasNonemptyDesignator`... or perhaps reverse the sense of it and name it `refersToCompleteObject` or similar? http://reviews.llvm.org/D12821 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits