Re: My Second OpenDocument patch

For the record: I just committed the patch. Daniel: feel free to address
some of Dom's suggestions in a follow up patch ;)

Thanks,
  Marc

On Wed, 2005-07-27 at 05:38 -0700, 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.
> 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 16:32:08 2005