Re: My Second OpenDocument patch

I'm satisfied with these answers. This patch can be
committed as-is. We'll work out any remaining issues
later.

Thanks,
Dom

--- Daniel d'Andrada Tenório de Carvalho
<daniel.carvalho@indt.org.br> wrote:

> 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
>
>

                
__________________________________
Yahoo! Mail for Mobile
Take Yahoo! Mail with you! Check email on your mobile phone.
http://mobile.yahoo.com/learn/mail
Received on Wed Jul 27 16:17:26 2005