[Piglit] [PATCH] framework: fix potential bug in function signatures

[Ilia Mirkin]

On Thu, Apr 17, 2014 at 11:36 AM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> It seems perfectly safe to do something like this:
> def foo(defaults=[]):
>      <stuff>
>
> But its not. Python will only initialize defaults once, and store it.
> Which means each time that foo is called with arguments defaults is
> changed. So when calling foo() more than once with arguments unexpected
> things can happen. This can create some very strange and hard to pin
> down bugs.

I don't think that's quite right. The problem is that

def foo(defaults=[]):
  ...

is the same as

x = []
def foo(defaults=x):
  ...

So if you modify "defaults" as part of your function, and it's not
passed in as an explicit argument, then future invocations of foo will
see the "modified" version if that argument wasn't supplied. It's a
disaster waiting to happen, but I'm pretty sure that the existing uses
were fine.

>
> Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> ---
>  framework/core.py | 14 ++++++++------
>  tests/igt.py      |  4 +++-
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/framework/core.py b/framework/core.py
> index 3979499..ac5be2b 100644
> --- a/framework/core.py
> +++ b/framework/core.py
> @@ -344,8 +344,8 @@ class TestrunResult:
>
>
>  class Environment:
> -    def __init__(self, concurrent=True, execute=True, include_filter=[],
> -                 exclude_filter=[], valgrind=False, dmesg=False, verbose=False):
> +    def __init__(self, concurrent=True, execute=True, include_filter=None,
> +                 exclude_filter=None, valgrind=False, dmesg=False, verbose=False):
>          self.concurrent = concurrent
>          self.execute = execute
>          self.filter = []
> @@ -361,10 +361,12 @@ class Environment:
>
>          This code uses re.compile to rebuild the lists and set self.filter
>          """
> -        for each in include_filter:
> -            self.filter.append(re.compile(each))
> -        for each in exclude_filter:
> -            self.exclude_filter.append(re.compile(each))
> +        if include_filter is not None:
> +            for each in include_filter:
> +                self.filter.append(re.compile(each))
> +        if exclude_filter is not None:
> +            for each in exclude_filter:
> +                self.exclude_filter.append(re.compile(each))

A little trick I like to use is

for each in include_filter or []:
  ...

But you don't have to do that, just an optional suggestion.

Either way,

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

>
>      def __iter__(self):
>          for key, values in self.__dict__.iteritems():
> diff --git a/tests/igt.py b/tests/igt.py
> index 9c700a9..766f543 100644
> --- a/tests/igt.py
> +++ b/tests/igt.py
> @@ -72,7 +72,9 @@ igtEnvironmentOk = checkEnvironment()
>  profile = TestProfile()
>
>  class IGTTest(Test):
> -    def __init__(self, binary, arguments=[]):
> +    def __init__(self, binary, arguments=None):
> +        if arguments is None:
> +            arguments = []
>          super(IGTTest, self).__init__(
>              [path.join(igtTestRoot, binary)] + arguments)
>
> --
> 1.9.2
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit