Re: My Second OpenDocument patch

Hi dom,

Dom Lachowicz wrote:
> Hi Daniel,
>
> I just looked through the patch, and it's pretty good.
> However, it is 4500 lines long, which is almost too
> big to grok. My comments here are mostly cosmetic and
> I just require some explaination of your design
> decisions. Thanks.
>
> 1) Please don't use setlocale(). Use the
> UT_LocaleTransactor class instead.

This is inherited code, I haven't touched on that. But I can fix it on
the next patch.

> 2) In a few places (at least OD_StartTag), you use
> pointer arrays instead of vectors. Why?

I tried to use the AbiWord UT vector class, but it doesn't fit my needs.
 I needed a slightly different control over the data.

> 2.a) In some of those instances, the data you're
> modelling is key=value, with "key" on the even
> entries, and "value" on the odd ones. Why did you use
> an array instead of a map?

I must be talking about the "UT_UTF8Stringbuf* m_pAttributes". I thought
  about using a hash map, but it would allocate and deallocate memory
overtime and I don't wanted it. I wanted to have a chunk of memory that
only grows, if needed, and that never shrinks. That means a buffer.

The OD_StartTag objects are reused to avoid "news" and "deletes" during
the document parsing. Because the stack formed by the parents of the
element being currently parsed goes up and down all the time and there
is a OD_StartTag for every parent element.

> 3) You don't need to use "inline" in your header
> files. The compiler will inline the function anyway.
> But it doesn't look necessary to inline the statements
> in the first place.

It won't do any harm either, and states your intentions more clearly. I
also done that way because I have seen "inline" in headers many times on
the AbiWord code. So I just followed the "standard".

> 4) You introduced a class called OD_StartTag for
> "performance" reasons. What was hurting the
> performance?

The objective of this class is to store the hierarchy of the parents of
the element being currently parsed. That info is useful in some places
and is very good for "sanity checks" and debuging.

It's a stack that goes up and down all the time during the document parsing.

That's a new feature, that kind of info wasn't available before. So,
those "performance reasons" regards to the implementation of the
class/functionality and not to its existence.

By the way, the next patch will do some modifications over the
implementation of this functionality (will remove this responsability
from do OpenDocument_StreamListenar abstract class to a proper
OD_ElementStack class).

Best Regards,
Daniel d'Andrada T. de Carvalho - INdT
Received on Wed Jul 27 15:57:00 2005