Re: My Second OpenDocument patch

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.
2) In a few places (at least OD_StartTag), you use
pointer arrays instead of vectors. Why?
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?
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.
4) You introduced a class called OD_StartTag for
"performance" reasons. What was hurting the
performance?

I'm sorry for being such a PITA :)

Best,
Dom

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

> Hi Dom,
>
> I got your point. You're right.
>
> So, I stripped of the "make the spacing between
> methods even" entries
> from the patch. But regarding to "better support to
> styles" and
> "increased modularization", they are very tigthly
> related and cannot be
> separated easily. I'm sorry for that.
>
> The new version of the patch is attached to this
> e-mail.
>
> Best Regards,
> Daniel d'Andrada T. de Carvalho - INdT
>
>
> Dom Lachowicz wrote:
> > Hi Daniel,
> >
> > 1) Please make the spacing between methods just
> one
> > line.
> > 2) Please don't include patches that do 3 separate
> > things. The most important bit here is "Improves
> style
> > import/export", and yet those changes are buried
> > between two other sets of changes. It only serves
> to
> > obfuscate the real work done.
> >
> > Thanks,
> > Dom
> >
> > --- Daniel d'Andrada Tenório de Carvalho
> > <daniel.carvalho@indt.org.br> wrote:
> >
> >
> >>Hi guys,
> >>
> >>Attached to this e-mail is my new patch for the
> >>OpenDocument plugin. It
> >>does the following (in order of importance):
> >>
> >>1 - Increases the modularization of the code,
> mainly
> >>regarding to styles.
> >>
> >>2 - Improves styles handling/import.
> >>
> >>3 - Makes the spacing between methods even (5
> lines)
> >>for most files. So,
> >>if you see entries on the patch just adding or
> >>removing blank lines
> >>don't go mad on me. :-)
> >>
> >>What do you guys think? Good for commit?
> >>
> >>My next patch (within some days) should add
> >>header/footer support.
> >>
> >>Best Regards,
> >>Daniel d'Andrada T. de Carvalho - INdT
>
>

                
____________________________________________________
Start your day with Yahoo! - make it your home page
http://www.yahoo.com/r/hs
 
Received on Wed Jul 27 14:41:39 2005