Re: [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 09, 2014 at 01:50:30PM +0000, Gore, Tim wrote:
> Some inline comments
> 
>   Tim
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, July 07, 2014 5:12 PM
> > To: Gore, Tim
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple
> > main
> > 
> > On Fri, Jun 27, 2014 at 03:15:36PM +0100, tim.gore@intel.com wrote:
> > > From: Tim Gore <tim.gore@intel.com>
> > >
> > > Quite a few single tests do not use the igt_simple_main macro because
> > > they want access to argc/argv. So change the igt_simple_main macro to
> > > pass these arguments through to the "__real_mainxxx" function, and
> > > change these tests to use the macro.
> > >
> > > Signed-off-by: Tim Gore <tim.gore@intel.com>
> > > ---
> > >  lib/igt_core.h                   |  8 ++++----
> 
>  
> > >  tests/gem_ctx_basic.c            |  6 +-----
> > >  tests/gem_exec_blt.c             |  5 +----
> > >  tests/gem_gtt_speed.c            |  5 +----
> > >  tests/gem_hang.c                 |  5 +----
> > >  tests/gem_render_copy.c          |  4 +---
> > >  tests/gem_render_linear_blits.c  |  5 +----
> > >  tests/gem_render_tiled_blits.c   |  5 +----
> > >  tests/gem_seqno_wrap.c           | 11 ++++-------
> > >  tests/gem_stress.c               |  5 +----
> > >  tests/gen3_mixed_blits.c         |  5 +----
> > >  tests/gen3_render_linear_blits.c |  5 +----
> > > tests/gen3_render_mixed_blits.c  |  5 +----
> > > tests/gen3_render_tiledx_blits.c |  5 +----
> > > tests/gen3_render_tiledy_blits.c |  5 +----
> > >  15 files changed, 21 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/lib/igt_core.h b/lib/igt_core.h index e252eba..9853e6b
> > > 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -176,13 +176,13 @@ void igt_simple_init(void);
> > >   * the test needs to parse additional cmdline arguments of its own.
> > >   */
> > >  #define igt_simple_main \
> > > -	static void igt_tokencat(__real_main, __LINE__)(void); \
> > > +	static void igt_tokencat(__real_main, __LINE__)(int argc, char
> > > +**argv); \
> > >  	int main(int argc, char **argv) { \
> > >  		igt_simple_init(); \
> > > -		igt_tokencat(__real_main, __LINE__)(); \
> > > -		exit(0); \
> > > +		igt_tokencat(__real_main, __LINE__)(argc, argv); \
> > > +		igt_exit(); \
> > >  	} \
> > > -	static void igt_tokencat(__real_main, __LINE__)(void) \
> > > +	static void igt_tokencat(__real_main, __LINE__)(int argc, char
> > > +**argv) \
> > 
> > Hm, I'd just add argc/argv to igt_simple_init like you do in the next patch and
> > update the relevant tests which have their own main. That would be more in
> > line with subtest-tests which have their own additional arguments, too.
> > 
> > Also having functions with magic/predefined arguments is a bit too much
> > magic for my taste. If we keep the signature of real_main as-is we'll avoid
> > (highly unlikely, but still) accidental name clashes.
> > 
> > And with my suggestion for patch 2 to just have a bare-bones argv parsing for
> > simple tests we also don't need to call igt_exit.
> > -Daniel
> 
>     My motivation here is mainly to reduce the amount of differentiation, so that
>     we can move away from having a different methodology for tests that want to
>     parse argv. Together with the next patch we move towards all tests having  the
>     same setup/initialisation. I believe that this will make the test suite easier to
>     maintain and add features to, such as common Doxygen/--help text.
>     I agree that the passing argc/v through to real_main is not ideal, but the whole
>     macro thing is not to my taste to be honest, for all the usual reasons. If we can
>    reduce the number of test "types" (single/multiple , parses argv/or not) it
>     should make the macros easier to maintain etc.

The problem is that simple tests without subtest do work slightly
differently. One option we could pursue instead is to redefine
igt_simple_main as

igt_main
	igt_subtest("basic")

but that will look a bit ugly in the test listenings. My proposal for this
patch here was to add (argc, argv) parameters to igt_simple_init and then
adjust just that function in patch 2. I don't see the upside of forcefully
converting the tests to use the igt_simple_main macro - we also have
subtest-tests with their open-coded main() function.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch