add default constructor to PendingTask; compilation fix for VS2012

add default constructor to PendingTask; compilation fix for VS2012

std::queue in TaskQueue wants a default ctor for its contained value in 2012.

R=willchan@chromium.org
BUG=143646
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160736

[willchan no longer on Chromium, 2012-10-08 21:59:44]

lgtm

[commit-bot: I haz the power, 2012-10-08 22:02:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/11095007/1

[commit-bot: I haz the power, 2012-10-08 22:58:35]

Change committed as 160736

[Ryan Sleevi, 2012-10-08 23:08:41]

https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc
File base/pending_task.cc (right):

https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc#n...
base/pending_task.cc:12: }
drive by: Isn't sequence_num uninitialized because you're providing a
user-defined constructor, even though it's POD? Should you be initializing it to
zero, "just in case"?

As for the default ctor - is that just because the lack of a move ctor?

[scottmg, 2012-10-08 23:27:13]

On 2012/10/08 23:08:41, Ryan Sleevi wrote:
> https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc
> File base/pending_task.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc#n...
> base/pending_task.cc:12: }
> drive by: Isn't sequence_num uninitialized because you're providing a
> user-defined constructor, even though it's POD? Should you be initializing it
to
> zero, "just in case"?

Well, sequence_num and nestable are definitely undefined. I lean towards not
initializing on the premise that it's ~ a bug workaround. But I'm happy to
change it if you prefer.

> 
> As for the default ctor - is that just because the lack of a move ctor?

Hmm, I don't know. I tried adding a declaration for:

PendingTask(PendingTask&& other);

but it still wants a default. Is the above wrong?

[Ryan Sleevi, 2012-10-09 02:44:51]

On 2012/10/08 23:27:13, scottmg wrote:
> On 2012/10/08 23:08:41, Ryan Sleevi wrote:
> > https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc
> > File base/pending_task.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc#n...
> > base/pending_task.cc:12: }
> > drive by: Isn't sequence_num uninitialized because you're providing a
> > user-defined constructor, even though it's POD? Should you be initializing
it
> to
> > zero, "just in case"?
> 
> Well, sequence_num and nestable are definitely undefined. I lean towards not
> initializing on the premise that it's ~ a bug workaround. But I'm happy to
> change it if you prefer.

I'm with you on it being a bug workaround, I just fear that it's quick and easy
for this bug workaround to become part of the API proper. For example, there's
no comment in the header saying "Don't use this, it's just for MSVS 2012", and
"if it compiles, it fits".

So yeah, +1 on providing a 'proper' initializer (to match the others), assuming
those are the only two values that can get insane.

> 
> > 
> > As for the default ctor - is that just because the lack of a move ctor?
> 
> Hmm, I don't know. I tried adding a declaration for:
> 
> PendingTask(PendingTask&& other);
> 
> but it still wants a default. Is the above wrong?

Hrm. I thought that the EmplaceConstructible would be suitable for MSVC.

It's odd that we don't run into this error in any previous versions of MSVC, nor
on other systems. AFAIK, C++11 doesn't require DefaultConstructible (for deque
or for vector, which queue/priority_queue are adapters for). Well - short of
calling .resize() explicitly, which I didn't think was happening here.

C++98 does require for both vector and queue that types be CopyConstructible -
and I wonder if the absence of a copy-ctor is triggering some sort of
DefaultConstructible+Assignable emulation for CopyConstructible.

I'll try to get an MSVC 2012 setup, but I suppose my near-term drive-by would be
that it may be worth adding a comment "don't use this" (and possibly provide
sane values for those that ignore the comment), and then we can work on removing
it.

[scottmg, 2012-10-09 03:57:34]

On 2012/10/09 02:44:51, Ryan Sleevi wrote:
> On 2012/10/08 23:27:13, scottmg wrote:
> > On 2012/10/08 23:08:41, Ryan Sleevi wrote:
> > >
https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc
> > > File base/pending_task.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/11095007/diff/1/base/pending_task.cc#n...
> > > base/pending_task.cc:12: }
> > > drive by: Isn't sequence_num uninitialized because you're providing a
> > > user-defined constructor, even though it's POD? Should you be initializing
> it
> > to
> > > zero, "just in case"?
> > 
> > Well, sequence_num and nestable are definitely undefined. I lean towards not
> > initializing on the premise that it's ~ a bug workaround. But I'm happy to
> > change it if you prefer.
> 
> I'm with you on it being a bug workaround, I just fear that it's quick and
easy
> for this bug workaround to become part of the API proper. For example, there's
> no comment in the header saying "Don't use this, it's just for MSVS 2012", and
> "if it compiles, it fits".

OK, follow up CL here: https://codereview.chromium.org/11066071/
And bug here: http://crbug.com/154744

> 
> So yeah, +1 on providing a 'proper' initializer (to match the others),
assuming
> those are the only two values that can get insane.
> 
> > 
> > > 
> > > As for the default ctor - is that just because the lack of a move ctor?
> > 
> > Hmm, I don't know. I tried adding a declaration for:
> > 
> > PendingTask(PendingTask&& other);
> > 
> > but it still wants a default. Is the above wrong?
> 
> Hrm. I thought that the EmplaceConstructible would be suitable for MSVC.
> 
> It's odd that we don't run into this error in any previous versions of MSVC,
nor
> on other systems. AFAIK, C++11 doesn't require DefaultConstructible (for deque
> or for vector, which queue/priority_queue are adapters for). Well - short of
> calling .resize() explicitly, which I didn't think was happening here.
> 
> C++98 does require for both vector and queue that types be CopyConstructible -
> and I wonder if the absence of a copy-ctor is triggering some sort of
> DefaultConstructible+Assignable emulation for CopyConstructible.
> 
> I'll try to get an MSVC 2012 setup, but I suppose my near-term drive-by would
be
> that it may be worth adding a comment "don't use this" (and possibly provide
> sane values for those that ignore the comment), and then we can work on
removing
> it.