[fpc-devel] Review of AVR patch for bug 31925

[Karoly Balogh (Charlie/SGR)]

Hi,

On Wed, 20 Sep 2017, Christo wrote:

> I have made an attempt at fixing an AVR related bug.  Since this is my
> first attempt at messing with the compiler, I would like a review and
> critique of my attempt.
> (...)
>
> Can anyone see any problems with this approach, or perhaps suggest a
> better approach to fix this bug?

I don't know a lot about AVR, and haven't actually tried your patch. But I
took a look at it.

Few things I see: I wouldn't do IFDEF CPUAVR in inc/generic and
inc/compproc. Use maybe IFDEF CPU8 and/or CPU16 instead. This code is
indeed generic, and will be useful on other limited CPU archs as well. Or
just follow the structure of other MUL helpers, and don't ifdef based on
the CPU width at all. The linker will (should) optimize away this code on
architectures where it is unnecessary.

Also, instead of copypasting the helper function call code, I'd think
about generalizing it, and extracting it to a private method of tcgavr,
but as I'm not the maintainer of the AVR cg, i'm not sure this is what
they would want there. :)

But in general I'd say it's the right approach, at least we used this
elsewhere too.

Charlie