Compilation of components_unittests with clang from Xcode broken |
||
Issue description
Compilation on iOS with the version of clang coming from Xcode (7.3 in that case) is broken with the following error:
../../components/autofill/core/browser/autofill_external_delegate_unittest.cc:139:22: error: default initialization of an object of const type 'const gfx::RectF' without a user-provided default constructor
const gfx::RectF element_bounds;
^
{}
../../components/autofill/core/browser/autofill_external_delegate_unittest.cc:588:20: error: default initialization of an object of const type 'const gfx::RectF' without a user-provided default constructor
const gfx::RectF element_bounds;
^
{}
2 errors generated.
I think this is caused by https://codereview.chromium.org/2051343002. Apparently the version of clang coming with Xcode does not like something in how the default constructor is declared.
Since the gfx::RectF bounds are unspecified when using the default constructor (as the float fields will be left uninitialized), a possible workaround is to use another constructor (since the code obviously does not care about the values of width/height/x/y of element_bounds).
,
Jun 16 2016
RectF has no float fields. It only has a PointF and a SizeF. Those, in turn, have user-defined default constructors that init all their fields. Since RectF has an "= default" null constructor, this should all work. I don't understand the error message here.
,
Jun 16 2016
Section 8.5 of the C++ standard is apparently relevant here, but the totality of the rules are a bit beyond me.
,
Jun 16 2016
I think the easiest workaround in this particular unittest is to inline the const default-constructed temps into the subsequent function calls, like: external_delegate_->OnQuery(query_id, FormData(), field, gfx::RectF()); But this still concerns me. I want to understand if we're wrong or the compiler is, because it seems to me that an "= default" constructor in this case ought to be good enough to allow this.
,
Jun 16 2016
I don't know why both version of the compiler don't agree on the rule. Looking at the rule in the standard it kinds of make sense. > If a program calls for the default initialization of an object of a > const-qualified type T, T shall be a class type with a user-provided > default constructor. If the object has no user-provided constructor (which is now the case as we use =default everywhere) then the object is a POD and uninitialized when constructed, right? If in addition to that the object is "const" then we have an uninitialized object that cannot be ever initialized, so it is probably unintended (what's the point of having a immutable uninitialized object, it has undetermined value, and using it is undefined behaviour). Inlining the object, changing it to non-const, or calling a non-default constructor are all valid changes. The last one ensure that we do not use uninitialized values in the rest of the code and has my personal preference. WDYT?
,
Jun 16 2016
As I said, this struct's members are aggregates that have user-provided default constructors that explicitly initialize all their fields. So it doesn't seem to me like an =default constructor for this struct can fail to call those constructors and leave the aggregates unconstructed. There shouldn't be able to be any uninitialized values in the struct. I must be misunderstanding something about POD types. Until I understand whether and how a RectF could ever have uninitialized values when default-constructed, I definitely don't think we should explicitly call a non-null constructor for this, because that basically is the same as saying that the default constructor doesn't actually work, which is a timebomb for the rest of the codebase if true. RectF() ought to be fine. And RectF r; ought to be fine too, IMO.
,
Jun 16 2016
Reading more on this. C++03 says "An aggregate is an array or a class (clause 9) with no user-declared constructors, no private or protected non-static data members (clause 11) ..." That means that RectF, PointF, and SizeF aren't aggregates; they all have a bunch of user-declared constructors, as well as private data members. It also says "A POD-struct is an aggregate class that has no non-static data members of type non-POD-struct..." None of these are aggregates, so they're all non-POD (RectF is a non-aggregate with non-aggregate members so it's doubly non-POD). The section 8.5 quote you reference in comment 5 refers, AFAICT, only to POD types. It certainly doesn't make sense to apply it to non-POD types; there's no reason they'd fail to be initialized. So I still think the compiler is wrong here (which is supported by the fact that no other compiler on our other platforms complained about this).
,
Jun 16 2016
It is possible that the compiler shipping with Xcode is wrong here. It is a bit older than the one used on the other platforms (clang ToT). It is only failing downstream because we only use that compiler for official builds. I'll do the workaround to get the compilation to pass in https://codereview.chromium.org/2060943008.
,
Jun 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7d77d2274c07224580b060a49399215ce0ebce1f commit 7d77d2274c07224580b060a49399215ce0ebce1f Author: sdefresne <sdefresne@chromium.org> Date: Thu Jun 16 12:13:08 2016 Fix compilation with clang shipped with Xcode. After https://codereview.chromium.org/2051343002, the version of clang shipped with Xcode gets confused and think that gfx::RectF is a POD type and warns about constructing a const POD value. Workaround this by passing a temporary non-const value instead. BUG= 620673 Review-Url: https://codereview.chromium.org/2060943008 Cr-Commit-Position: refs/heads/master@{#400135} [modify] https://crrev.com/7d77d2274c07224580b060a49399215ce0ebce1f/components/autofill/core/browser/autofill_external_delegate_unittest.cc
,
Jun 16 2016
|
||
►
Sign in to add a comment |
||
Comment 1 by sdefresne@chromium.org
, Jun 16 2016