Move helper_gmock.h from ChromFrame subrtee to testing directory....

Move helper_gmock.h from ChromFrame subrtee to testing directory.


BUG=none
TEST=builds ok

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

[stoyan, 2009-10-22 21:56:59]

ping?

[awong, 2009-10-23 02:57:12]

Sorry...was OOO for most of today.  Will get to this tomorrow morning.
-A

On Thu, Oct 22, 2009 at 2:56 PM, <stoyan@chromium.org> wrote:

> ping?
>
>
> http://codereview.chromium.org/295049
>

[awong, 2009-10-24 00:32:16]

Mostly style comments. This will be cool to have. :)

http://codereview.chromium.org/295049/diff/1/3
File testing/gmock.gyp (right):

http://codereview.chromium.org/295049/diff/1/3#newcode36
Line 36: 'helper_gmock.h',
indentation?

http://codereview.chromium.org/295049/diff/1/2
File testing/helper_gmock.h (right):

http://codereview.chromium.org/295049/diff/1/2#newcode3
Line 3: // found in the LICENSE file.
Newline.

http://codereview.chromium.org/295049/diff/1/2#newcode5
Line 5: #define TESTING_HELPER_GMOCK_H_
newline.

http://codereview.chromium.org/295049/diff/1/2#newcode7
Line 7: // all of its syntactic beauty. Classes and helper functions could be
used as
could -> can

http://codereview.chromium.org/295049/diff/1/2#newcode10
Line 10: // arguments (like Callback) - hence the name. :-)
Add a newline after paragraphs, here and in the rest of the comment.

http://codereview.chromium.org/295049/diff/1/2#newcode11
Line 11: // DispatchToMethod supporting two sets of arguments -
I think you mean

DispatchToMethod supports two sets of arguments: pre-bound (P) and call-time
(C).  The arguments as well as the return type are templatized.

http://codereview.chromium.org/295049/diff/1/2#newcode13
Line 13: // It will also try to call the selected method even if provided
pre-bound
It -> DispatchToMethod.

The antecedent is ambiguous here.

http://codereview.chromium.org/295049/diff/1/2#newcode20
Line 20: // of T* and T** when we invoke CreateFunctor to match the EXPECT_CALL
style.
Can you add a simple example of the usage for both pre-bound and call-time
arguments?

http://codereview.chromium.org/295049/diff/1/2#newcode26
Line 26: // 0 - 0
Should this be added into the testing namespace?

http://codereview.chromium.org/295049/diff/1/2#newcode255
Line 255: // Mutant holds pre-bound arguments (like Task) and like Callback
"Mutant holds pre-bound arguments like Task.  Like Callback 
it"

http://codereview.chromium.org/295049/diff/1/2#newcode256
Line 256: // allows call-time arguments. You bind pointer to the object at
creation time.
You bind pointer -> You bind a pointer

http://codereview.chromium.org/295049/diff/1/2#newcode271
Line 271: Method meth_;
avoid abbreviations?

http://codereview.chromium.org/295049/diff/1/2#newcode276
Line 276: // MutantLateBind is like Mutant, but you bind pointer to
"you bind pointer to pointer" -> "you bind a pointer to a pointer"

http://codereview.chromium.org/295049/diff/1/2#newcode277
Line 277: // pointer to the object. This way you can create actions for object
"for an object"

http://codereview.chromium.org/295049/diff/1/2#newcode278
Line 278: // that is not yet created (has only storage for pointer to it).
"for a pointer"

http://codereview.chromium.org/295049/diff/1/2#newcode287
Line 287: // MutantRunner implementation
Add a period.

http://codereview.chromium.org/295049/diff/1/2#newcode1019
Line 1019: #endif
Add a newline here.

[stoyan, 2009-10-26 18:58:05]

http://codereview.chromium.org/295049/diff/1/3
File testing/gmock.gyp (right):

http://codereview.chromium.org/295049/diff/1/3#newcode36
Line 36: 'helper_gmock.h',
On 2009/10/24 00:32:17, awong wrote:
> indentation?

Done.

http://codereview.chromium.org/295049/diff/1/2
File testing/helper_gmock.h (right):

http://codereview.chromium.org/295049/diff/1/2#newcode3
Line 3: // found in the LICENSE file.
On 2009/10/24 00:32:17, awong wrote:
> Newline.

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode5
Line 5: #define TESTING_HELPER_GMOCK_H_
On 2009/10/24 00:32:17, awong wrote:
> newline.

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode7
Line 7: // all of its syntactic beauty. Classes and helper functions could be
used as
On 2009/10/24 00:32:17, awong wrote:
> could -> can

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode10
Line 10: // arguments (like Callback) - hence the name. :-)
On 2009/10/24 00:32:17, awong wrote:
> Add a newline after paragraphs, here and in the rest of the comment.

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode11
Line 11: // DispatchToMethod supporting two sets of arguments -
On 2009/10/24 00:32:17, awong wrote:
> I think you mean
> 
> DispatchToMethod supports two sets of arguments: pre-bound (P) and call-time
> (C).  The arguments as well as the return type are templatized.

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode13
Line 13: // It will also try to call the selected method even if provided
pre-bound
On 2009/10/24 00:32:17, awong wrote:
> It -> DispatchToMethod.
> 
> The antecedent is ambiguous here.

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode20
Line 20: // of T* and T** when we invoke CreateFunctor to match the EXPECT_CALL
style.
On 2009/10/24 00:32:17, awong wrote:
> Can you add a simple example of the usage for both pre-bound and call-time
> arguments?

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode26
Line 26: // 0 - 0
On 2009/10/24 00:32:17, awong wrote:
> Should this be added into the testing namespace?

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode255
Line 255: // Mutant holds pre-bound arguments (like Task) and like Callback
On 2009/10/24 00:32:17, awong wrote:
> "Mutant holds pre-bound arguments like Task.  Like Callback 
> it"

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode256
Line 256: // allows call-time arguments. You bind pointer to the object at
creation time.
On 2009/10/24 00:32:17, awong wrote:
> You bind pointer -> You bind a pointer

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode271
Line 271: Method meth_;
On 2009/10/24 00:32:17, awong wrote:
> avoid abbreviations?

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode276
Line 276: // MutantLateBind is like Mutant, but you bind pointer to
On 2009/10/24 00:32:17, awong wrote:
> "you bind pointer to pointer" -> "you bind a pointer to a pointer"

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode277
Line 277: // pointer to the object. This way you can create actions for object
On 2009/10/24 00:32:17, awong wrote:
> "for an object"

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode278
Line 278: // that is not yet created (has only storage for pointer to it).
On 2009/10/24 00:32:17, awong wrote:
> "for a pointer"

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode287
Line 287: // MutantRunner implementation
On 2009/10/24 00:32:17, awong wrote:
> Add a period.

Done.

http://codereview.chromium.org/295049/diff/1/2#newcode1019
Line 1019: #endif
On 2009/10/24 00:32:17, awong wrote:
> Add a newline here.

Done.

[awong, 2009-10-26 20:21:07]

More style nits. :)

Should be the last pass though.

-Your friendly readability reviewer.

http://codereview.chromium.org/295049/diff/7001/8001
File testing/helper_gmock.h (right):

http://codereview.chromium.org/295049/diff/7001/8001#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Is this copyright correct?  I though this code was new as of 2009.

http://codereview.chromium.org/295049/diff/7001/8001#newcode6
Line 6: #define TESTING_HELPER_GMOCK_H_
helper_gmock.h seems over general.  gmock_mutant.h seems more correct.

http://codereview.chromium.org/295049/diff/7001/8001#newcode78
Line 78: //
No more than 2 new-lines.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_White...

http://codereview.chromium.org/295049/diff/7001/8001#newcode83
Line 83: //   MOCK_METHOD1(void, DemiurgeCreated(Demiurge*));
Demiurge huh? Cute. :)

http://codereview.chromium.org/295049/diff/7001/8001#newcode88
Line 88: //  }
indentation.

http://codereview.chromium.org/295049/diff/7001/8001#newcode90
Line 90: //  Demiurge* demiurge;
indent.

http://codereview.chromium.org/295049/diff/7001/8001#newcode102
Line 102: 
One newline is probably enough.

http://codereview.chromium.org/295049/diff/7001/8001#newcode108
Line 108: 
1 newline.

http://codereview.chromium.org/295049/diff/7001/8001#newcode342
Line 342: typename PreBound, typename Params>
align with first typename.

http://codereview.chromium.org/295049/diff/7001/8001#newcode432
Line 432: CreateFunctor(T* obj, R (T::*method)()) {
I wonder if we should make the MutantFunctor constructor above private, and move
this into the class as a static method?

Or do you think that would be too annoying to use?

http://codereview.chromium.org/295049/diff/7001/8001#newcode445
Line 445: MutantRunner<R, Tuple0> *t = new MutantLateObjectBind<R, T,
Breaking up the template specialization parameters is hard to read. Breaking the
newline after the = is probably easier to read.

MutantRunner<R, Tuple0> = 
    new MutantLateObjectBind<R, T, R (T::*)(), Tuple0, Tuple0>
       (obj, method, MakeTyple());

If it gets too long, I'd prefer to wrap the specializations to keep the concept
of the "type" separate from the arguments.  Example:

MutantRunner<R, Tuple0> = 
   new MutantLateObjectBind<R, T, R (T::*)(),
                           Tuple0, Tuple0>
       (obj, method, MakeTyple());


Though, I wonder...is it worth typedefing these various binders?  Something like
the various Tuple1, Tuple2, Tuple3, etc.? Might not be wroth the trouble
tough...

http://codereview.chromium.org/295049/diff/7001/8001#newcode1105
Line 1105: }  // namespace
Should be

// namespace testing

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

[stoyan, 2009-10-26 22:00:26]

http://codereview.chromium.org/295049/diff/7001/8001
File testing/helper_gmock.h (right):

http://codereview.chromium.org/295049/diff/7001/8001#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/10/26 20:21:07, awong wrote:
> Is this copyright correct?  I though this code was new as of 2009.

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode6
Line 6: #define TESTING_HELPER_GMOCK_H_
On 2009/10/26 20:21:07, awong wrote:
> helper_gmock.h seems over general.  gmock_mutant.h seems more correct.

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode78
Line 78: //
On 2009/10/26 20:21:07, awong wrote:
> No more than 2 new-lines.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_White...

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode88
Line 88: //  }
On 2009/10/26 20:21:07, awong wrote:
> indentation.

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode90
Line 90: //  Demiurge* demiurge;
On 2009/10/26 20:21:07, awong wrote:
> indent.

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode102
Line 102: 
On 2009/10/26 20:21:07, awong wrote:
> One newline is probably enough.

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode108
Line 108: 
On 2009/10/26 20:21:07, awong wrote:
> 1 newline.

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode342
Line 342: typename PreBound, typename Params>
On 2009/10/26 20:21:07, awong wrote:
> align with first typename.

Done.

http://codereview.chromium.org/295049/diff/7001/8001#newcode432
Line 432: CreateFunctor(T* obj, R (T::*method)()) {
Looks like this will cause a lot of typing just to select the function you want:
MutantFunctor<int, Tuple2<int, int> >::CreateFunctor(.. )
effectively removing the benefits of parameters deduction.

http://codereview.chromium.org/295049/diff/7001/8001#newcode445
Line 445: MutantRunner<R, Tuple0> *t = new MutantLateObjectBind<R, T,
I guess this code is not going to be read often and I'm using script to generate
all these so typedefs are not of much use.
Aligned according the recommendation.

http://codereview.chromium.org/295049/diff/7001/8001#newcode1105
Line 1105: }  // namespace
On 2009/10/26 20:21:07, awong wrote:
> Should be
> 
> // namespace testing
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

Done.

[awong, 2009-10-26 22:09:34]

LGTM

BTW, if you're using a script to generate it, we should check that in too and
refer to it in the comments so someone knows how to regenerate.

[stoyan, 2009-10-26 22:18:40]

Sure, I will go through the python style guide first :)

[awong, 2009-10-26 22:20:06]

heh... the python linter (forgot what it is called) is your friend. :) :)

On Mon, Oct 26, 2009 at 3:18 PM, <stoyan@chromium.org> wrote:

> Sure, I will go through the python style guide first :)
>
>
> http://codereview.chromium.org/295049
>