Subject: More Bidi patch comments - everybody please read
From: Jesper Skov (jskov@redhat.com)
Date: Sat Dec 30 2000 - 15:04:31 CST
OK, some overall (hard) questions:
o What does it mean to split the carret? I haven't tried the code,
but will tomorrow. Would be good with an explanation in words
though.
Is it necessary? It's causing most of the gross changes. In
particular I'd be unhappy (helping to) maintain code that is so
full of #ifdef BIDI_ENABLED.
o If it _is_ necessary (which it probably is), what would be the code
size and performance overhead of always using the (x, y, x2, y2)
form of function signatures - then the #ifdefs would be limited to
semantics inside function bodies.
o Finally, what is the code size and performance overhead of just
making BiDi The Way AbiWord works? Get rid of all the conditional
code that makes maintainment and understanding harder.
FYI I have not had time to read throug for semantics. I'll try to do
it tomorrow, but I suspect I don't understand enough of the existing
code to understand your changes. We'll see...
Tomas, below a few specific comments for you primarily.
Cheers,
Jesper
/****/
Typedefs, maybe put them in a new file fmt_Types.h, similar to the
pt_Types.h in ptbl.
/****/
+ NB!!! _createMapOfRuns has to be called before these functions are called;
+ the call to _createMapOfRuns was not included inside, because sometimes
+ we call _getRunVis(Log)Indx repeatedly inside of another memeber function, i.e.,
+ inside a loop -- it is more efficient to call _createMapOfRuns
+ only once prior to the first call of these funcitons */
Yes, there may be a little performance gain. But it's so little it's
hardly worth clobbering the code in such an ugly way. Please move the
checks/optional-calls into those functions instead.
If you really want speed, instead inline those functions. You'll
replace a subroutine call+return with a forward branch. Faster, yet
easier to maintain.
I'm sure if you were to profile it, it wouldn't mean much either
way. Even so, code optimizations like these are best left until the
algorithms are properly optimized - and that gets harder to do when
the code is harder to read/understand.
/****/
You have a few of these:
+ if(!UT_stricmp(pszDirection, "rtl")) m_iDirection = 1;
+ else m_iDirection = 0;
(and similar). I suspect some people would probably prefer it was more
according to the style guide.
This archive was generated by hypermail 2b25 : Sat Dec 30 2000 - 15:04:36 CST