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 <[EMAIL PROTECTED]> > > > > 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 <[EMAIL PROTECTED]> > > > > 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 _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
