Split tests into individual files. Got rid of globals by converting them to classes.

Split tests into individual files. Got rid of globals by converting them to classes.

[piman, 2010-05-20 23:00:39]

OO-design wise, I think it works. I could see separating TestFunc into a
separate class because after all you /could/ imagine having tests that don't
bench in terms of iterations, but I don't think it's worth spending too much
time on it.
The other comments are mostly style nits. I haven't given a very close look at
all the individual tests, I think it's mostly cut-n-paste of the old ones, right
?

http://codereview.chromium.org/2123013/diff/3001/4002
File client/deps/glbench/src/attributefetchtest.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4002#newcode148
client/deps/glbench/src/attributefetchtest.cc:148: }
add a // namespace glbench after closing } (here and all the other files)

http://codereview.chromium.org/2123013/diff/3001/4003
File client/deps/glbench/src/bench.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4003#newcode7
client/deps/glbench/src/bench.cc:7: 
I would consider moving these 2 functions, as well as RunTest into testbase.cc
(and their prototypes into testbase.h)
It helps having all the test infrastructure at the same place to understand the
relationship between TestBase::TestFunc, TestBase::Run, RunTest and Bench

http://codereview.chromium.org/2123013/diff/3001/4005
File client/deps/glbench/src/drawfunc.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4005#newcode1
client/deps/glbench/src/drawfunc.cc:1: // Copyright (c) 2010 The Chromium OS
Authors. All rights reserved.
This file should be testbase.cc, or alternatively the class definition should be
in drawfunc.h

http://codereview.chromium.org/2123013/diff/3001/4007
File client/deps/glbench/src/main.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4007#newcode123
client/deps/glbench/src/main.cc:123: }
delete all tests ?

http://codereview.chromium.org/2123013/diff/3001/4008
File client/deps/glbench/src/main.h (right):

http://codereview.chromium.org/2123013/diff/3001/4008#newcode90
client/deps/glbench/src/main.h:90: const char *name, float coefficient, bool
inverse);
shouldn't these 2 go into the glbench namespace as well ?

http://codereview.chromium.org/2123013/diff/3001/4014
File client/deps/glbench/src/testbase.h (right):

http://codereview.chromium.org/2123013/diff/3001/4014#newcode18
client/deps/glbench/src/testbase.h:18: virtual bool TestFunc(int) = 0;
you should name the parameter, and it would be good to add some documentation
here. Why 2 functions TestFunc and Run ? Which one calls the other one ?

http://codereview.chromium.org/2123013/diff/3001/4014#newcode27
client/deps/glbench/src/testbase.h:27: void FillRateTestNormal(const char* name,
float coeff=1.f);
default values for arguments are disallowed by the style guide.

http://codereview.chromium.org/2123013/diff/3001/4014#newcode40
client/deps/glbench/src/testbase.h:40: GLsizei count_;
What is count ? (add doc)

http://codereview.chromium.org/2123013/diff/3001/4014#newcode51
client/deps/glbench/src/testbase.h:51: TestBase*
GetWindowManagerCompositingTest(bool scissor);
I think it would be cleaner to move these to a separate "all_tests.h" or
something.

http://codereview.chromium.org/2123013/diff/3001/4016
File client/deps/glbench/src/utils.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4016#newcode192
client/deps/glbench/src/utils.cc:192: }
add a  // namespace glbench

http://codereview.chromium.org/2123013/diff/3001/4019
File client/deps/glbench/src/windowmanagercompositingtest.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4019#newcode14
client/deps/glbench/src/windowmanagercompositingtest.cc:14: const float
screen_scale_factor = 1e6f * (WINDOW_WIDTH * WINDOW_HEIGHT) /
screen_scale_factor -> kScreenScaleFactor

http://codereview.chromium.org/2123013/diff/3001/4019#newcode34
client/deps/glbench/src/windowmanagercompositingtest.cc:34:
DISALLOW_COPY_AND_ASSIGN(WindowManagerCompositingTest);
should be last in the class

http://codereview.chromium.org/2123013/diff/3001/4019#newcode137
client/deps/glbench/src/windowmanagercompositingtest.cc:137: const char
*basic_texture_vertex_shader =
All these strings should be kBasicTextureVertexShader etc.
Also, start-on-the-left

http://codereview.chromium.org/2123013/diff/3001/4019#newcode154
client/deps/glbench/src/windowmanagercompositingtest.cc:154: GLuint
texture_buffer) {
indentation (could collapse on 1 line)

http://codereview.chromium.org/2123013/diff/3001/4019#newcode156
client/deps/glbench/src/windowmanagercompositingtest.cc:156:
InitShaderProgram(basic_texture_vertex_shader,
indentation (should be +4 spaces), or collapse on 1 line

[Alexey Marinichev, 2010-05-21 02:17:34]

http://codereview.chromium.org/2123013/diff/3001/4002
File client/deps/glbench/src/attributefetchtest.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4002#newcode148
client/deps/glbench/src/attributefetchtest.cc:148: }
On 2010/05/20 23:00:39, piman wrote:
> add a // namespace glbench after closing } (here and all the other files)

Done.

http://codereview.chromium.org/2123013/diff/3001/4003
File client/deps/glbench/src/bench.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4003#newcode7
client/deps/glbench/src/bench.cc:7: 
On 2010/05/20 23:00:39, piman wrote:
> I would consider moving these 2 functions, as well as RunTest into testbase.cc
> (and their prototypes into testbase.h)
> It helps having all the test infrastructure at the same place to understand
the
> relationship between TestBase::TestFunc, TestBase::Run, RunTest and Bench

I was thinking I'd separate data processing from the actual tests -- in theory
we might different kinds of regressions for different tests.  But that doesn't
buy us much at the moment and we can always split them again later.

http://codereview.chromium.org/2123013/diff/3001/4005
File client/deps/glbench/src/drawfunc.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4005#newcode1
client/deps/glbench/src/drawfunc.cc:1: // Copyright (c) 2010 The Chromium OS
Authors. All rights reserved.
On 2010/05/20 23:00:39, piman wrote:
> This file should be testbase.cc, or alternatively the class definition should
be
> in drawfunc.h
I was thinking there could be more of these mixin-like functions in the future,
they might not necessarily belong to testbase.

http://codereview.chromium.org/2123013/diff/3001/4007
File client/deps/glbench/src/main.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4007#newcode123
client/deps/glbench/src/main.cc:123: }
On 2010/05/20 23:00:39, piman wrote:
> delete all tests ?
It felt strange -- I don't call new, but I call delete. Maybe that's OK though,
all those Get* functions are effectively virtual constructors.

http://codereview.chromium.org/2123013/diff/3001/4008
File client/deps/glbench/src/main.h (right):

http://codereview.chromium.org/2123013/diff/3001/4008#newcode90
client/deps/glbench/src/main.h:90: const char *name, float coefficient, bool
inverse);
On 2010/05/20 23:00:39, piman wrote:
> shouldn't these 2 go into the glbench namespace as well ?

This was an unfinished thought about separating tests and collecting their
results.  Will move everything to testbase.h.

http://codereview.chromium.org/2123013/diff/3001/4014
File client/deps/glbench/src/testbase.h (right):

http://codereview.chromium.org/2123013/diff/3001/4014#newcode18
client/deps/glbench/src/testbase.h:18: virtual bool TestFunc(int) = 0;
On 2010/05/20 23:00:39, piman wrote:
> you should name the parameter, and it would be good to add some documentation
> here. Why 2 functions TestFunc and Run ? Which one calls the other one ?

Done.

http://codereview.chromium.org/2123013/diff/3001/4014#newcode27
client/deps/glbench/src/testbase.h:27: void FillRateTestNormal(const char* name,
float coeff=1.f);
On 2010/05/20 23:00:39, piman wrote:
> default values for arguments are disallowed by the style guide.

Done.

http://codereview.chromium.org/2123013/diff/3001/4014#newcode40
client/deps/glbench/src/testbase.h:40: GLsizei count_;
On 2010/05/20 23:00:39, piman wrote:
> What is count ? (add doc)

Done.

http://codereview.chromium.org/2123013/diff/3001/4014#newcode51
client/deps/glbench/src/testbase.h:51: TestBase*
GetWindowManagerCompositingTest(bool scissor);
On 2010/05/20 23:00:39, piman wrote:
> I think it would be cleaner to move these to a separate "all_tests.h" or
> something.

Done.

http://codereview.chromium.org/2123013/diff/3001/4016
File client/deps/glbench/src/utils.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4016#newcode192
client/deps/glbench/src/utils.cc:192: }
On 2010/05/20 23:00:39, piman wrote:
> add a  // namespace glbench

Done.

http://codereview.chromium.org/2123013/diff/3001/4019
File client/deps/glbench/src/windowmanagercompositingtest.cc (right):

http://codereview.chromium.org/2123013/diff/3001/4019#newcode14
client/deps/glbench/src/windowmanagercompositingtest.cc:14: const float
screen_scale_factor = 1e6f * (WINDOW_WIDTH * WINDOW_HEIGHT) /
On 2010/05/20 23:00:39, piman wrote:
> screen_scale_factor -> kScreenScaleFactor

Done.

http://codereview.chromium.org/2123013/diff/3001/4019#newcode34
client/deps/glbench/src/windowmanagercompositingtest.cc:34:
DISALLOW_COPY_AND_ASSIGN(WindowManagerCompositingTest);
On 2010/05/20 23:00:39, piman wrote:
> should be last in the class

Done.

http://codereview.chromium.org/2123013/diff/3001/4019#newcode137
client/deps/glbench/src/windowmanagercompositingtest.cc:137: const char
*basic_texture_vertex_shader =
On 2010/05/20 23:00:39, piman wrote:
> All these strings should be kBasicTextureVertexShader etc.
> Also, start-on-the-left

Done.

http://codereview.chromium.org/2123013/diff/3001/4019#newcode154
client/deps/glbench/src/windowmanagercompositingtest.cc:154: GLuint
texture_buffer) {
On 2010/05/20 23:00:39, piman wrote:
> indentation (could collapse on 1 line)

Done.

http://codereview.chromium.org/2123013/diff/3001/4019#newcode156
client/deps/glbench/src/windowmanagercompositingtest.cc:156:
InitShaderProgram(basic_texture_vertex_shader,
On 2010/05/20 23:00:39, piman wrote:
> indentation (should be +4 spaces), or collapse on 1 line

Done.

[piman, 2010-05-21 02:29:03]

LGTM

[piman, 2010-05-21 02:34:29]

On Thu, May 20, 2010 at 7:17 PM, <amarinichev@chromium.org> wrote:

>
> http://codereview.chromium.org/2123013/diff/3001/4002
> File client/deps/glbench/src/attributefetchtest.cc (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4002#newcode148
> client/deps/glbench/src/attributefetchtest.cc:148: }
> On 2010/05/20 23:00:39, piman wrote:
>
>> add a // namespace glbench after closing } (here and all the other
>>
> files)
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4003
> File client/deps/glbench/src/bench.cc (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4003#newcode7
> client/deps/glbench/src/bench.cc:7:
> On 2010/05/20 23:00:39, piman wrote:
>
>> I would consider moving these 2 functions, as well as RunTest into
>>
> testbase.cc
>
>> (and their prototypes into testbase.h)
>> It helps having all the test infrastructure at the same place to
>>
> understand the
>
>> relationship between TestBase::TestFunc, TestBase::Run, RunTest and
>>
> Bench
>
> I was thinking I'd separate data processing from the actual tests -- in
> theory we might different kinds of regressions for different tests.  But
> that doesn't buy us much at the moment and we can always split them
> again later.



I think that'd make more sense with a separate interface that has TestFunc.
Base interface: run a test. More refined interface: the test has a notion of
iteration and we want to find the linear response part.


>
>
> http://codereview.chromium.org/2123013/diff/3001/4005
> File client/deps/glbench/src/drawfunc.cc (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4005#newcode1
> client/deps/glbench/src/drawfunc.cc:1: // Copyright (c) 2010 The
> Chromium OS Authors. All rights reserved.
> On 2010/05/20 23:00:39, piman wrote:
>
>> This file should be testbase.cc, or alternatively the class definition
>>
> should be
>
>> in drawfunc.h
>>
> I was thinking there could be more of these mixin-like functions in the
> future, they might not necessarily belong to testbase.


I don't mind either way, but the idea is to match the .h with the .cc for
easier navigation.


>
>
> http://codereview.chromium.org/2123013/diff/3001/4007
> File client/deps/glbench/src/main.cc (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4007#newcode123
> client/deps/glbench/src/main.cc:123: }
> On 2010/05/20 23:00:39, piman wrote:
>
>> delete all tests ?
>>
> It felt strange -- I don't call new, but I call delete. Maybe that's OK
> though, all those Get* functions are effectively virtual constructors.


They're essentially factories. The other possibility is to use scoped_ptr.


>
> http://codereview.chromium.org/2123013/diff/3001/4008
> File client/deps/glbench/src/main.h (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4008#newcode90
> client/deps/glbench/src/main.h:90: const char *name, float coefficient,
> bool inverse);
> On 2010/05/20 23:00:39, piman wrote:
>
>> shouldn't these 2 go into the glbench namespace as well ?
>>
>
> This was an unfinished thought about separating tests and collecting
> their results.  Will move everything to testbase.h.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4014
> File client/deps/glbench/src/testbase.h (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4014#newcode18
> client/deps/glbench/src/testbase.h:18: virtual bool TestFunc(int) = 0;
> On 2010/05/20 23:00:39, piman wrote:
>
>> you should name the parameter, and it would be good to add some
>>
> documentation
>
>> here. Why 2 functions TestFunc and Run ? Which one calls the other one
>>
> ?
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4014#newcode27
> client/deps/glbench/src/testbase.h:27: void FillRateTestNormal(const
> char* name, float coeff=1.f);
> On 2010/05/20 23:00:39, piman wrote:
>
>> default values for arguments are disallowed by the style guide.
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4014#newcode40
> client/deps/glbench/src/testbase.h:40: GLsizei count_;
> On 2010/05/20 23:00:39, piman wrote:
>
>> What is count ? (add doc)
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4014#newcode51
> client/deps/glbench/src/testbase.h:51: TestBase*
> GetWindowManagerCompositingTest(bool scissor);
> On 2010/05/20 23:00:39, piman wrote:
>
>> I think it would be cleaner to move these to a separate "all_tests.h"
>>
> or
>
>> something.
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4016
> File client/deps/glbench/src/utils.cc (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4016#newcode192
> client/deps/glbench/src/utils.cc:192: }
> On 2010/05/20 23:00:39, piman wrote:
>
>> add a  // namespace glbench
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4019
> File client/deps/glbench/src/windowmanagercompositingtest.cc (right):
>
> http://codereview.chromium.org/2123013/diff/3001/4019#newcode14
> client/deps/glbench/src/windowmanagercompositingtest.cc:14: const float
> screen_scale_factor = 1e6f * (WINDOW_WIDTH * WINDOW_HEIGHT) /
> On 2010/05/20 23:00:39, piman wrote:
>
>> screen_scale_factor -> kScreenScaleFactor
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4019#newcode34
> client/deps/glbench/src/windowmanagercompositingtest.cc:34:
> DISALLOW_COPY_AND_ASSIGN(WindowManagerCompositingTest);
> On 2010/05/20 23:00:39, piman wrote:
>
>> should be last in the class
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4019#newcode137
> client/deps/glbench/src/windowmanagercompositingtest.cc:137: const char
> *basic_texture_vertex_shader =
> On 2010/05/20 23:00:39, piman wrote:
>
>> All these strings should be kBasicTextureVertexShader etc.
>> Also, start-on-the-left
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4019#newcode154
> client/deps/glbench/src/windowmanagercompositingtest.cc:154: GLuint
> texture_buffer) {
> On 2010/05/20 23:00:39, piman wrote:
>
>> indentation (could collapse on 1 line)
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/diff/3001/4019#newcode156
> client/deps/glbench/src/windowmanagercompositingtest.cc:156:
> InitShaderProgram(basic_texture_vertex_shader,
> On 2010/05/20 23:00:39, piman wrote:
>
>> indentation (should be +4 spaces), or collapse on 1 line
>>
>
> Done.
>
>
> http://codereview.chromium.org/2123013/show
>