Re: [PATCH v2] Implement benchmark script in python

[Mike Frysinger <vapier at gentoo dot org>]

On Friday 27 December 2013 10:04:45 Siddhesh Poyarekar wrote:

largely looks good.  mostly nits below.

when it comes to declaring coding style, i'm not sure the GNU project has one 
already for python (since they try to push Guile/scheme).  GDB has been 
growing a python code base, but a scan of that tree doesn't turn up anything 
either :(.

looking around the glibc tree, i think adding a section to manual/maint.texi 
would be the best bet.

> --- a/scripts/bench.pl
> +++ /dev/null

since we're not meant to review this, the -D flag would help

> --- /dev/null
> +++ b/scripts/bench.py
> @@ -0,0 +1,305 @@
> +#!/usr/bin/python
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +# This file is part of the GNU C Library.
> +#
> +# The GNU C Library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +#
> +# The GNU C Library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with the GNU C Library; if not, see
> +# <http://www.gnu.org/licenses/>.
> +"""Benchmark program generator script

prefer a newline above that docstring 

> +def gen_source(func, directives, all_vals):
> +    """Generate source for the function
> +
> +    Generate the C source for the function from the values and
> +    directives.
> +
> +    Args:
> +      func: The function name
> +      directives: A dictionary of directives applicable to this function
> +      all_vals: A dictionary input values
> +
> +    """

no need for that trailing blank line in the docstring.  goes for all funcs in 
this file that i can see.

> +    for header in directives['include-sources']:
> +        print('#include \"%s\"' % header)

are those \ still needed ?

> +    for k, i in zip(all_vals.keys(), itertools.count()):
> +        vals = all_vals[k]

could do:
	for (k, v), i in zip(all_vals.items(), itertools.count()):

> +        # Members for the variants structure list that we will
> +        # print later.
> +        variants.append('  {\"%s(%s)\", %d, in%d},' % (func, k, len(vals),

same here -- are those \ still needed ?

> +    directives = {'name': '',
> +                  'args': [],
> +                  'includes': [],
> +                  'include-sources': [],
> +                  'ret': ''}

in cases like this, it's neater to uncuddle entirely:
	directives = {
		'name': '',
		'args': [],
		'includes': [],
		'include-sources': [],
		'ret': '',
	}

(i use tabs just because it's easier to type)

> +            for line in f:
> +                line = line.rstrip()
> +
> +                # Look for directives and parse it if found.
> +                if line.startswith('##'):
> +                    try:
> +                        d_name, d_val = line[2:].split(':', 1)
> +                        d_name = d_name.strip()
> +                        d_val = d_val.strip()
> +                        directives[d_name] = _process_directive(d_name,
> +                    except (IndexError, KeyError):
> +                        die('Invalid directive: %s' % line[2:])
> +
> +                # Skip blank lines and comments.
> +                line = line.split('#', 1)[0].rstrip()
> +                if not line:
> +                    continue

do you really need that initial rstrip() at the top of this loop ?  the 
name/val parsing calls strip themselves, and this final line parser calls 
rstrip() too.

> +def die(msg):
> +    """Exit with an error
> +
> +    Prints an error message to the standard error stream and exits with
> +    a non-zero status.
> +
> +    Args:
> +      msg: The error message to print to standard error.
> +
> +    """
> +    sys.stderr.write('%s\n' % msg)

alternative:
	print('%s\n' % msg, file=sys.stderr)

> +def main(args):
> +    """Main function
> +
> +    Use the first command line argument as function name and parse its
> +    input file to generate C source that calls the function repeatedly
> +    for the input.
> +
> +    Args:
> +      args: The command line arguments with the program name dropped.
> +
> +    Returns:
> +      1 on error and 0 on success.

the os module provides EX_* constants ...

> +    if len(args) != 1:
> +        print('Usage: %s <function>' % sys.argv[0])
> +        return 1

... os.EX_USAGE

> +    directives, all_vals = parse_file(args[0])
> +    gen_source(args[0], directives, all_vals)
> +    return 0

... os.EX_OK

> --- /dev/null
> +++ b/scripts/pylintrc

any reason for that vs generating a default one and tweaking it ?  see 
pylint's --generate-rcfile option.

would be helpful i think to add a scripts/pylint wrapper:
	#!/bin/sh
	exec pylint --rcfile "${0%/*}/pylintrc" "$@"

that way people can do:
	./scripts/pylint */*.py
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.