New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 620673 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Compilation of components_unittests with clang from Xcode broken

Project Member Reported by sdefresne@chromium.org, Jun 16 2016

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).
 
Cc: pkasting@chromium.org
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.
Section 8.5 of the C++ standard is apparently relevant here, but the totality of the rules are a bit beyond me.
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.
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?
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.
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).
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.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment