Cleanup: IWYU for base/time.h in sync/.

Cleanup: IWYU for base/time.h in sync/.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161059

[akalin, 2012-10-09 21:22:27]

LGTM

http://codereview.chromium.org/11028086/diff/10002/sync/internal_api/public/b...
File sync/internal_api/public/base_node.h (right):

http://codereview.chromium.org/11028086/diff/10002/sync/internal_api/public/b...
sync/internal_api/public/base_node.h:92: const base::Time& GetModificationTime()
const;
doesn't a forward-declaration suffice for this?

But this should really just return base::Time directly.  Can you change it?

[Lei Zhang, 2012-10-09 22:38:40]

https://chromiumcodereview.appspot.com/11028086/diff/10002/sync/internal_api/...
File sync/internal_api/public/base_node.h (right):

https://chromiumcodereview.appspot.com/11028086/diff/10002/sync/internal_api/...
sync/internal_api/public/base_node.h:92: const base::Time& GetModificationTime()
const;
On 2012/10/09 21:22:27, akalin wrote:
> doesn't a forward-declaration suffice for this?
> 
> But this should really just return base::Time directly.  Can you change it?

I thought we want to return a const ref when possible. I can forward decl.

[akalin, 2012-10-09 23:06:50]

On 2012/10/09 22:38:40, Lei Zhang wrote:
>
https://chromiumcodereview.appspot.com/11028086/diff/10002/sync/internal_api/...
> File sync/internal_api/public/base_node.h (right):
> 
>
https://chromiumcodereview.appspot.com/11028086/diff/10002/sync/internal_api/...
> sync/internal_api/public/base_node.h:92: const base::Time&
GetModificationTime()
> const;
> On 2012/10/09 21:22:27, akalin wrote:
> > doesn't a forward-declaration suffice for this?
> > 
> > But this should really just return base::Time directly.  Can you change it?
> 
> I thought we want to return a const ref when possible. I can forward decl.

Not for small types.  time.h says:

// These classes are represented as only a 64-bit value, so they can be
// efficiently passed by value.

[Lei Zhang, 2012-10-09 23:17:02]

On 2012/10/09 23:06:50, akalin wrote:
> On 2012/10/09 22:38:40, Lei Zhang wrote:
> >
>
https://chromiumcodereview.appspot.com/11028086/diff/10002/sync/internal_api/...
> > File sync/internal_api/public/base_node.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11028086/diff/10002/sync/internal_api/...
> > sync/internal_api/public/base_node.h:92: const base::Time&
> GetModificationTime()
> > const;
> > On 2012/10/09 21:22:27, akalin wrote:
> > > doesn't a forward-declaration suffice for this?
> > > 
> > > But this should really just return base::Time directly.  Can you change
it?
> > 
> > I thought we want to return a const ref when possible. I can forward decl.
> 
> Not for small types.  time.h says:
> 
> // These classes are represented as only a 64-bit value, so they can be
> // efficiently passed by value.

Not sure if that's true, isn't there more to the Time class than just a 64-bit
value? In any case, assuming it's true, see patch set 3.

[akalin, 2012-10-09 23:25:44]

On 2012/10/09 23:17:02, Lei Zhang wrote:
> Not sure if that's true, isn't there more to the Time class than just a 64-bit
> value? In any case, assuming it's true, see patch set 3.

I don't think so.  Looks like it's still just a 64-bit value.

[akalin, 2012-10-09 23:25:53]

https://codereview.chromium.org/11028086/diff/10002/sync/internal_api/public/...
File sync/internal_api/public/base_node.h (right):

https://codereview.chromium.org/11028086/diff/10002/sync/internal_api/public/...
sync/internal_api/public/base_node.h:92: const base::Time& GetModificationTime()
const;
On 2012/10/09 22:38:40, Lei Zhang wrote:
> On 2012/10/09 21:22:27, akalin wrote:
> > doesn't a forward-declaration suffice for this?
> > 
> > But this should really just return base::Time directly.  Can you change it?
> 
> I thought we want to return a const ref when possible. I can forward decl.

What about the .cc file?

[Lei Zhang, 2012-10-09 23:46:17]

On 2012/10/09 23:25:44, akalin wrote:
> On 2012/10/09 23:17:02, Lei Zhang wrote:
> > Not sure if that's true, isn't there more to the Time class than just a
64-bit
> > value? In any case, assuming it's true, see patch set 3.
> 
> I don't think so.  Looks like it's still just a 64-bit value.

The ctor/dtor/vtable don't count?

re: cc file. Were you referring to sync/internal_api/base_node.cc? I forgot to
add time.h to sync/internal_api/public/base_node.h, and fixed that in patch set
4. It's enough to just add it to the .h file and not the .cc file as well.

[akalin, 2012-10-10 00:05:04]

On 2012/10/09 23:46:17, Lei Zhang wrote:
> The ctor/dtor/vtable don't count?

It doesn't have a vtable.  It does have a ctor and (implicit) dtor/copy ctor but
that only blocks it from being a static variable, and not from being passed
around by value.

> 
> re: cc file. Were you referring to sync/internal_api/base_node.cc? I forgot to
> add time.h to sync/internal_api/public/base_node.h, and fixed that in patch
set
> 4. It's enough to just add it to the .h file and not the .cc file as well.

Yeah, I was referring to base_node.cc.  If you change GetModificationTime() to
return a base::Time, you need to update that file as well.

[Lei Zhang, 2012-10-10 00:12:18]

Umm, ya, done. I'm having a special moment here.

[akalin, 2012-10-10 00:18:34]

LGTM!

[commit-bot: I haz the power, 2012-10-10 03:35:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/11028086/7008

[commit-bot: I haz the power, 2012-10-10 05:53:31]

Change committed as 161059