[poppler] Releasing RC1 one week later

[Albert Astals Cid]

A Dissabte 08 Març 2008, Carlos Garcia Campos va escriure:
> El sáb, 08-03-2008 a las 21:20 +1100, Brad Hards escribió:
> > On Saturday 08 March 2008 08:29:03 pm Carlos Garcia Campos wrote:
> > > El vie, 07-03-2008 a las 20:03 +1100, Brad Hards escribió:
> > > > On Monday 03 March 2008 09:25:10 pm Carlos Garcia Campos wrote:
> > > > > I've started to look at the optional content stuff and I've
> > > > > realized there are some things  implemented in the qt4 frontend
> > > > > that should be in the core, like the order array and radio buttons
> > > > > stuff. I've changed/fixed other minor issues. Bradh, could you have
> > > > > a look at these patches, please?
> > > > >
> > > > > http://people.freedesktop.org/~carlosgc/poppler-optcontent.diff
> > > >
> > > > I haven't tried / tested any of them, but I'll still offer my
> > > > opinion, based on initial review.
> > >
> > > we can just keep both interfaces, getOCGs() returns the
> > > optionalContentGroups and getOrderArray() the array object, and
> > > getOrderedOCGs or something like that would return the list of optional
> > > content already ordered.
> >
> > Please don't do this. Just use the existing interface. Then people who
> > don't want to present the UI part don't pay for parsing.
>
> They won't, since they can still use the getOCGs instead of
> getOrderedOCGs. I don't see the problem.
>
> > If you do both, then the original argument for common code doesn't hold
> > anyway.
> >
> > > > The foo() -> getFoo() rename is fine (not standard for Qt, but looks
> > > > more standard for poppler),
> > >
> > > yes, that's consistent with poppler core code.
> > >
> > > >  but please don't put the implementation in the header.
> > >
> > > ok, no problem, but just out of curiosity, why?
> >
> > Because it limits ability to fix things later, and because it is ugly. I
> > shouldn't have done any, but almost certainly did. It isn't that big a
> > deal.
> >
> > > > From: Carlos Garcia Campos <carlosgc at gnome.org>
> > > > Date: Sun, 2 Mar 2008 19:43:15 +0100
> > > > Subject: [PATCH] Use an array instead of a list for optional content
> > > > groups Sorry, I don't understand what you are trying to accomplish
> > > > with this. Seems gratuitous. Can you explain more?
> > >
> > > We don't really need a list when we already know the size of the list.
> > > An array it's much more efficient than a list and it's also more
> > > consistent with the poppler code.
> >
> > Did you do any measurements of the impact?
>
> Well, accessing a list is O(n) while an array is O(1) and you avoid a
> lot allocation/deallocations of memory. Looking at the GooList
> implementation for n <= 8 it's almost the same than using a list.
>
> > I really don't care to much, but make sure you don't break the Qt4
> > bindings.
>
> It's not a huge change, changes needed in the qt4 side would be quite
> trivial.
>
> > > > >From 157d763c3e0e5432cce4da391fab99d0854a63f5 Mon Sep 17 00:00:00
> > > > > 2001
> > > >
> > > > From: Carlos Garcia Campos <carlosgc at gnome.org>
> > > > Date: Sun, 2 Mar 2008 19:52:37 +0100
> > > > Subject: [PATCH] Fix memory leaks
> > > > I don't see the leaks. Again, can you explain why this change is
> > > > needed?
> > >
> > > m_orderArray and m_rBGroupsArray are never freed. By using objects
> > > instead of pointers you avoid allocations/deallocations too.
> >
> > Seems like an excessive change. Why not just free it in the destructor?
>
> Again, because you avoid alloc/dealloc more than  needed.

Can we please try avoid the silliness of discussing if we are winning an 
alloc/dealloc in code that is completely not a hot path?

I would care more about how much easier or not it's to understand a solution 
and another (which i won't comment since i have to admit i have not read 
patches nor original code) but discussing if we are going to with something 
like 0.00001ms on a code that gets executed once every 5000 miles makes me a 
bit sad.

Albert