public inbox for gentoo-catalyst@lists.gentoo.org
 help / color / mirror / Atom feed
* [gentoo-catalyst] [rfc] simplifying arch classes
@ 2011-06-27  5:15 Matt Turner
  2011-06-27 17:41 ` Sebastian Pipping
  0 siblings, 1 reply; 5+ messages in thread
From: Matt Turner @ 2011-06-27  5:15 UTC (permalink / raw
  To: gentoo-catalyst

The arch class structure ({alpha,amd64,mips,hppa,etc}.py files) are a
lot of typing for the small number of attributes they synthesize. This
makes them prone to typing errors.

Consider mips.py. It supports big and little endian; mips 1, 3, 4,
loongson, cobalt; o32, n32, n64, multilib ABIs. Almost every
combination of these attributes exists as a hard-coded class, leaving
us with 24 builder and 5 abstract base classes.

Wouldn't it be simpler to pass in some information into an arch_mips
class and let it generate the requested attributes. Something like
this:

class arch_mips(generic):
    "MIPS class"
    def __init__(self, Olevel, arch, additional_cflags, include_workarounds):
        generic.__init__(self)

        self.settings["CFLAGS"] = "-O" + Olevel
        self.settings["CFLAGS"] += " -march=" + arch
        if additional_cflags != "":
            self.settings["CFLAGS"] += " " + additional_cflags
        if include_workarounds:
            if arch == "mips3":
                self.settings["CFLAGS"] += " -mfix-r4000 -mfix-r4400"
            elif arch == "r4000" or arch == "r4k":
                self.settings["CFLAGS"] += " -mfix-r4000"
            elif arch == "r4300":
                self.settings["CFLAGS"] += " -mfix-r4300"
            elif arch == "r10000" or arch == "r10k":
                self.settings["CFLAGS"] += " -mfix-r10000"
        self.settings["CFLAGS"] += " -pipe"

Clearly a simplistic and incomplete example, but it should be enough
to understand the idea.

Thanks,
Matt



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [gentoo-catalyst] [rfc] simplifying arch classes
  2011-06-27  5:15 [gentoo-catalyst] [rfc] simplifying arch classes Matt Turner
@ 2011-06-27 17:41 ` Sebastian Pipping
  2011-06-27 17:58   ` Matt Turner
  0 siblings, 1 reply; 5+ messages in thread
From: Sebastian Pipping @ 2011-06-27 17:41 UTC (permalink / raw
  To: gentoo-catalyst

On 06/27/2011 07:15 AM, Matt Turner wrote:
> class arch_mips(generic):
>     "MIPS class"
>     def __init__(self, Olevel, arch, additional_cflags, include_workarounds):
>         generic.__init__(self)
> 
>         self.settings["CFLAGS"] = "-O" + Olevel
>         self.settings["CFLAGS"] += " -march=" + arch
>         if additional_cflags != "":
>             self.settings["CFLAGS"] += " " + additional_cflags
>         if include_workarounds:
>             if arch == "mips3":
>                 self.settings["CFLAGS"] += " -mfix-r4000 -mfix-r4400"
>             elif arch == "r4000" or arch == "r4k":
>                 self.settings["CFLAGS"] += " -mfix-r4000"
>             elif arch == "r4300":
>                 self.settings["CFLAGS"] += " -mfix-r4300"
>             elif arch == "r10000" or arch == "r10k":
>                 self.settings["CFLAGS"] += " -mfix-r10000"
>         self.settings["CFLAGS"] += " -pipe"

Thoughts:

 - How are you going to ensure that such refactoring keeps all
   ~50 cases working without writing 50 explicit, data-duplicating
   test cases?  Would you be willing to write these?

 - The code above adds flexibility but is less obvious than
   the current code.  So while it improves on one aspect
   it worsens on another.

 - Such refactoring would have to be done on both 2.x and 3.x branches.
   Better wait until we are clear on their future.

Best,



Sebastian



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [gentoo-catalyst] [rfc] simplifying arch classes
  2011-06-27 17:41 ` Sebastian Pipping
@ 2011-06-27 17:58   ` Matt Turner
  2011-06-27 18:18     ` Sebastian Pipping
  0 siblings, 1 reply; 5+ messages in thread
From: Matt Turner @ 2011-06-27 17:58 UTC (permalink / raw
  To: gentoo-catalyst

On Mon, Jun 27, 2011 at 1:41 PM, Sebastian Pipping <sping@gentoo.org> wrote:
> On 06/27/2011 07:15 AM, Matt Turner wrote:
>> class arch_mips(generic):
>>     "MIPS class"
>>     def __init__(self, Olevel, arch, additional_cflags, include_workarounds):
>>         generic.__init__(self)
>>
>>         self.settings["CFLAGS"] = "-O" + Olevel
>>         self.settings["CFLAGS"] += " -march=" + arch
>>         if additional_cflags != "":
>>             self.settings["CFLAGS"] += " " + additional_cflags
>>         if include_workarounds:
>>             if arch == "mips3":
>>                 self.settings["CFLAGS"] += " -mfix-r4000 -mfix-r4400"
>>             elif arch == "r4000" or arch == "r4k":
>>                 self.settings["CFLAGS"] += " -mfix-r4000"
>>             elif arch == "r4300":
>>                 self.settings["CFLAGS"] += " -mfix-r4300"
>>             elif arch == "r10000" or arch == "r10k":
>>                 self.settings["CFLAGS"] += " -mfix-r10000"
>>         self.settings["CFLAGS"] += " -pipe"
>
> Thoughts:
>
>  - How are you going to ensure that such refactoring keeps all
>   ~50 cases working without writing 50 explicit, data-duplicating
>   test cases?  Would you be willing to write these?

There seems to be an implicit assumption that the current code has
some kind of working test cases. :)

This is certainly not the case. Let me be clear, mistakes in the
current code come from having the same CFLAG, CHOST, etc strings
duplicated in many places. Refactoring the code would allow us to
catch mistakes like
http://git.overlays.gentoo.org/gitweb/?p=proj/catalyst.git;a=commit;h=db4323146ce27362948de6eab57e1dbe28240bde
much more quickly.

It seems to me that test coverage would be much simpler if the classes
were refactored, since various combinations would use nearly identical
code paths.

>  - The code above adds flexibility but is less obvious than
>   the current code.  So while it improves on one aspect
>   it worsens on another.

Perhaps... I'm not sure I'd agree. I'm not sure 24 classes with only
slight variations between them lends itself to being obvious.

>  - Such refactoring would have to be done on both 2.x and 3.x branches.
>   Better wait until we are clear on their future.

Yes, of course.

Thanks,
Matt



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [gentoo-catalyst] [rfc] simplifying arch classes
  2011-06-27 17:58   ` Matt Turner
@ 2011-06-27 18:18     ` Sebastian Pipping
  2011-06-27 18:24       ` Matt Turner
  0 siblings, 1 reply; 5+ messages in thread
From: Sebastian Pipping @ 2011-06-27 18:18 UTC (permalink / raw
  To: gentoo-catalyst

On 06/27/2011 07:58 PM, Matt Turner wrote:
> There seems to be an implicit assumption that the current code has
> some kind of working test cases. :)

I'm aware that I'm asking for test cases in context that seems to lack
proper testing.


> This is certainly not the case. Let me be clear, mistakes in the
> current code come from having the same CFLAG, CHOST, etc strings
> duplicated in many places. Refactoring the code would allow us to
> catch mistakes like
> http://git.overlays.gentoo.org/gitweb/?p=proj/catalyst.git;a=commit;h=db4323146ce27362948de6eab57e1dbe28240bde
> much more quickly.
> 
> It seems to me that test coverage would be much simpler if the classes
> were refactored, since various combinations would use nearly identical
> code paths.

It would make some code pathes being taken more often but still leave
the "leafes" ontouched without a test for each leaf.  Right?

What could work though is a throw-away test for refactoring only, say
writing a piece of code making a text file listing all combination of
CFLAGS offered from targets.  If after the refactoring you get the very
same text file out, that's a good indicator.  Is the idea clear?

Best,



Sebastian



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [gentoo-catalyst] [rfc] simplifying arch classes
  2011-06-27 18:18     ` Sebastian Pipping
@ 2011-06-27 18:24       ` Matt Turner
  0 siblings, 0 replies; 5+ messages in thread
From: Matt Turner @ 2011-06-27 18:24 UTC (permalink / raw
  To: gentoo-catalyst

On Mon, Jun 27, 2011 at 2:18 PM, Sebastian Pipping <sping@gentoo.org> wrote:
>> This is certainly not the case. Let me be clear, mistakes in the
>> current code come from having the same CFLAG, CHOST, etc strings
>> duplicated in many places. Refactoring the code would allow us to
>> catch mistakes like
>> http://git.overlays.gentoo.org/gitweb/?p=proj/catalyst.git;a=commit;h=db4323146ce27362948de6eab57e1dbe28240bde
>> much more quickly.
>>
>> It seems to me that test coverage would be much simpler if the classes
>> were refactored, since various combinations would use nearly identical
>> code paths.
>
> It would make some code pathes being taken more often but still leave
> the "leafes" ontouched without a test for each leaf.  Right?
>
> What could work though is a throw-away test for refactoring only, say
> writing a piece of code making a text file listing all combination of
> CFLAGS offered from targets.  If after the refactoring you get the very
> same text file out, that's a good indicator.  Is the idea clear?

Yep, that should work and wouldn't be very much work. That seems like
a good idea.

Thanks!
Matt



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-06-27 18:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-27  5:15 [gentoo-catalyst] [rfc] simplifying arch classes Matt Turner
2011-06-27 17:41 ` Sebastian Pipping
2011-06-27 17:58   ` Matt Turner
2011-06-27 18:18     ` Sebastian Pipping
2011-06-27 18:24       ` Matt Turner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox