Fix comparisons

Fix comparisons. The type returned cc::Layer::transform_origin() is gfx::Point3F not gfx::PointF.

Follow-up from http://crrev.com/274990.

TBR=chrishtr@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275028

[darin (slow to review), 2014-06-05 06:21:56]

It seems like the string compares were unnecessary and error-prone. Better to
just compare gfx::Point3F objects directly for type-safety.

[darin (slow to review), 2014-06-05 06:22:51]

Committed patchset #2 manually as r275028 (tree was closed).

[enne (OOO), 2014-06-05 16:52:54]

String compares have much better output when they fail, which is why they were
done this way here (and elsewhere).  It might be better to have an
EXPECT_POINT3_EQ function instead that converts and then ToStrings.

[darin (slow to review), 2014-06-09 16:05:04]

On 2014/06/05 16:52:54, enne wrote:
> String compares have much better output when they fail, which is why they were
> done this way here (and elsewhere).  It might be better to have an
> EXPECT_POINT3_EQ function instead that converts and then ToStrings.

Oh, sorry. I suppose I should have just landed patch set 1. I figured making it
more typesafe was worth it to avoid more issues like the one I was fixing. Is
it possible to improve 

Rather than another macro, perhaps it is possible to improve the way gtest
outputs
Point3F? Can that be done by implementing operator<< or something like that?

[jamesr, 2014-06-09 16:09:50]

On 2014/06/09 16:05:04, darin wrote:
> On 2014/06/05 16:52:54, enne wrote:
> > String compares have much better output when they fail, which is why they
were
> > done this way here (and elsewhere).  It might be better to have an
> > EXPECT_POINT3_EQ function instead that converts and then ToStrings.
> 
> Oh, sorry. I suppose I should have just landed patch set 1. I figured making
it
> more typesafe was worth it to avoid more issues like the one I was fixing. Is
> it possible to improve 
> 
> Rather than another macro, perhaps it is possible to improve the way gtest
> outputs
> Point3F? Can that be done by implementing operator<< or something like that?

Yes, that's the normal way to do gtest output.  The downside is that puts the
operator<< implementation in production code - I haven't seen any way to have it
exist only in test code.  I haven't checked but I suspect the stream stuff in
the header is pretty big, even if it will be (mostly) dropped down to just one
function by the linker.

[darin (slow to review), 2014-06-09 16:10:57]

inline the function? that should make it test only, right? you can also use
the UNIT_TEST #ifdef in header files to guard against its use when
compiling non-test .cc files.

-darin


On Mon, Jun 9, 2014 at 9:09 AM, <jamesr@chromium.org> wrote:

> On 2014/06/09 16:05:04, darin wrote:
>
>> On 2014/06/05 16:52:54, enne wrote:
>> > String compares have much better output when they fail, which is why
>> they
>>
> were
>
>> > done this way here (and elsewhere).  It might be better to have an
>> > EXPECT_POINT3_EQ function instead that converts and then ToStrings.
>>
>
>  Oh, sorry. I suppose I should have just landed patch set 1. I figured
>> making
>>
> it
>
>> more typesafe was worth it to avoid more issues like the one I was
>> fixing. Is
>> it possible to improve
>>
>
>  Rather than another macro, perhaps it is possible to improve the way gtest
>> outputs
>> Point3F? Can that be done by implementing operator<< or something like
>> that?
>>
>
> Yes, that's the normal way to do gtest output.  The downside is that puts
> the
> operator<< implementation in production code - I haven't seen any way to
> have it
> exist only in test code.  I haven't checked but I suspect the stream stuff
> in
> the header is pretty big, even if it will be (mostly) dropped down to just
> one
> function by the linker.
>
> https://codereview.chromium.org/314053002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-06-09 16:12:30]

Also, since operator<< can just be a global function, you could put its
definition in a "test_utils" file that is only included by tests and only
linked in test builds.

-Darin


On Mon, Jun 9, 2014 at 9:10 AM, Darin Fisher <darin@chromium.org> wrote:

> inline the function? that should make it test only, right? you can also
> use the UNIT_TEST #ifdef in header files to guard against its use when
> compiling non-test .cc files.
>
> -darin
>
>
> On Mon, Jun 9, 2014 at 9:09 AM, <jamesr@chromium.org> wrote:
>
>> On 2014/06/09 16:05:04, darin wrote:
>>
>>> On 2014/06/05 16:52:54, enne wrote:
>>> > String compares have much better output when they fail, which is why
>>> they
>>>
>> were
>>
>>> > done this way here (and elsewhere).  It might be better to have an
>>> > EXPECT_POINT3_EQ function instead that converts and then ToStrings.
>>>
>>
>>  Oh, sorry. I suppose I should have just landed patch set 1. I figured
>>> making
>>>
>> it
>>
>>> more typesafe was worth it to avoid more issues like the one I was
>>> fixing. Is
>>> it possible to improve
>>>
>>
>>  Rather than another macro, perhaps it is possible to improve the way
>>> gtest
>>> outputs
>>> Point3F? Can that be done by implementing operator<< or something like
>>> that?
>>>
>>
>> Yes, that's the normal way to do gtest output.  The downside is that puts
>> the
>> operator<< implementation in production code - I haven't seen any way to
>> have it
>> exist only in test code.  I haven't checked but I suspect the stream
>> stuff in
>> the header is pretty big, even if it will be (mostly) dropped down to
>> just one
>> function by the linker.
>>
>> https://codereview.chromium.org/314053002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-06-09 16:15:30]

An inline function performing ostream operations will be really massive - I'd
worry about blowing the linker out.  The test_utils thing might work, I haven't
tried.

[darin (slow to review), 2014-06-09 16:20:12]

On 2014/06/09 16:15:30, jamesr wrote:
> An inline function performing ostream operations will be really massive - I'd
> worry about blowing the linker out.  The test_utils thing might work, I
haven't
> tried.

Hmm... these classes already have a .ToString() method that does most of the
work.

[jamesr, 2014-06-09 16:59:43]

On 2014/06/09 16:20:12, darin wrote:
> On 2014/06/09 16:15:30, jamesr wrote:
> > An inline function performing ostream operations will be really massive -
I'd
> > worry about blowing the linker out.  The test_utils thing might work, I
> haven't
> > tried.
> 
> Hmm... these classes already have a .ToString() method that does most of the
> work.

Yes, but that's out-of-line and only requires #including <string>, which most
translation units will have anyway.  A stream-based operator<< would require
including at least <ostream>.  Since the style guide bans streams in general, I
doubt most translation units will have it.  On my linux box, just adding
#include <ostream> increases the post-preprocessing size of
ui/gfx/geometry/point3_f.cc by 34% (17555 lines to 23559 lines).

[darin (slow to review), 2014-06-09 17:26:44]

I see. Well, I guess there are three options:

1) Status-quo string compares. Sometimes encounter mismatch issues like the
one I fixed here.
2) Define operator<< inline, but guard the include for ostream and the
operator<< definition with UNIT_TEST #define.
3) Create a separate "test_utils" .h and .cc for operator<< that would only
be included and linked against by tests.

I think you all should decide which way you want to go. I'm a fan of #2 or
#3 since type safety seems like a good thing, and it allows you to type a
bit less code.

-Darin


On Mon, Jun 9, 2014 at 9:59 AM, <jamesr@chromium.org> wrote:

> On 2014/06/09 16:20:12, darin wrote:
>
>> On 2014/06/09 16:15:30, jamesr wrote:
>> > An inline function performing ostream operations will be really massive
>> -
>>
> I'd
>
>> > worry about blowing the linker out.  The test_utils thing might work, I
>> haven't
>> > tried.
>>
>
>  Hmm... these classes already have a .ToString() method that does most of
>> the
>> work.
>>
>
> Yes, but that's out-of-line and only requires #including <string>, which
> most
> translation units will have anyway.  A stream-based operator<< would
> require
> including at least <ostream>.  Since the style guide bans streams in
> general, I
> doubt most translation units will have it.  On my linux box, just adding
> #include <ostream> increases the post-preprocessing size of
> ui/gfx/geometry/point3_f.cc by 34% (17555 lines to 23559 lines).
>
> https://codereview.chromium.org/314053002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-06-09 17:27:33]

On Mon, Jun 9, 2014 at 1:26 PM, Darin Fisher <darin@chromium.org> wrote:

> I see. Well, I guess there are three options:
>
> 1) Status-quo string compares. Sometimes encounter mismatch issues like
> the one I fixed here.
> 2) Define operator<< inline, but guard the include for ostream and the
> operator<< definition with UNIT_TEST #define.
> 3) Create a separate "test_utils" .h and .cc for operator<< that would
> only be included and linked against by tests.
>
> I think you all should decide which way you want to go. I'm a fan of #2 or
> #3 since type safety seems like a good thing, and it allows you to type a
> bit less code.
>

I'd love to see #3.


>
> -Darin
>
>
> On Mon, Jun 9, 2014 at 9:59 AM, <jamesr@chromium.org> wrote:
>
>> On 2014/06/09 16:20:12, darin wrote:
>>
>>> On 2014/06/09 16:15:30, jamesr wrote:
>>> > An inline function performing ostream operations will be really
>>> massive -
>>>
>> I'd
>>
>>> > worry about blowing the linker out.  The test_utils thing might work, I
>>> haven't
>>> > tried.
>>>
>>
>>  Hmm... these classes already have a .ToString() method that does most of
>>> the
>>> work.
>>>
>>
>> Yes, but that's out-of-line and only requires #including <string>, which
>> most
>> translation units will have anyway.  A stream-based operator<< would
>> require
>> including at least <ostream>.  Since the style guide bans streams in
>> general, I
>> doubt most translation units will have it.  On my linux box, just adding
>> #include <ostream> increases the post-preprocessing size of
>> ui/gfx/geometry/point3_f.cc by 34% (17555 lines to 23559 lines).
>>
>> https://codereview.chromium.org/314053002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.