Fix for style change crash, and code cleanup

[Maciej Stachowiak]

On Oct 2, 2003, at 5:23 PM, Dirk Mueller wrote:

> On Thursday 02 October 2003 21:02, Maciej Stachowiak wrote:
>
>> I fixed this by refactoring the code to put most of the global
>> variables in a struct allocated on the stack, and pass around a
>> reference to the struct. Seems much cleaner (if somewhat wordier) this
>> way.
>
> I don't think that things like
>
> -    ++eor;
> +    bidi.eor.increment( bidi );
>
> can be called a code cleanup.

I think you picked an unfair example. Most of the changes replace 
things like "last" with "bidi.last". See below for comment about the 
increment change.

> What would be correct is to actually create a
> class with those member variables, and turn the static helper 
> functions into
> members of this BidiState class.

I think this is a good idea and I will do it as a next step. However, 
while using a class is even better than using a struct, I still think 
using a struct is way better than using global variables, especially 
since it fixes a crashing bug. There's also two limitations to the 
class approach:

1) operator++ still has to be changed to a method that takes an 
argument, because it depends on what used to be global statel. So the 
.increment( bidi ) would just turn into .increment( this ).

2) several of the affected functions in bidi.cpp are already member 
functions of RenderBlock, so they can't also be member functions of 
BidiState. We'll need to either leave BidiState's fields public or else 
provide inline getters and setters.

Any suggestions on these points?


> In addition it would cut down the size of your patch by at least 80% 
> and make
> it somehow possible to merge for me in less than 5 hours of manual 
> conflict
> resolution. It would also ease further code cleanups.

Hey, it didn't even take me 5 hours to code it, so I'd be surprised if 
it took 5 hours to apply. :-)

Regards,
Maciej