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

[Dylan Baker]

On Thursday, April 17, 2014 11:45:25 Ilia Mirkin wrote:
> 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.

Right. I'll fix the commit message.

> 
> > 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 []:

I definitely like that better.

>   ...
> 
> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140417/8353ef77/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140417/8353ef77/attachment-0001.sig>