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

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

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=db4323146ce27362948de6eab5 7e1dbe28240bde
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

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=db4323146ce27362948de6eab5 7e1dbe28240bde
> 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

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=db4323146ce27362948de6eab5 7e1dbe28240bde
>> 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