[poppler] 13 commits - fofi/FoFiType1C.cc glib/poppler-action.cc glib/poppler-document.cc glib/poppler-page.cc makefile.vc poppler/ArthurOutputDev.cc poppler/CairoOutputDev.cc poppler/CharCodeToUnicode.cc poppler/Error.cc poppler/Gfx.cc poppler/G

[Jeff Muizelaar]

On Mon, Sep 24, 2007 at 12:17:26PM -0700, Krzysztof Kowalczyk wrote:
> On 9/24/07, Jeff Muizelaar <jeff at infidigm.net> wrote:
> > One of the reasons a lot of the warnings were kept is too keep the
> > source code closer to the xpdf upstream to make merging easier. I
> > haven't looked over these commits thoroughly yet to see how invasive
> > they are, but keep that in mind when changing code that isn't specific
> > to poppler.
> 
> I understand the need to keep the code unchanged to make merges easier.
> 
> On the downside, it does preclude making meaningful improvements to the code.
> 
> To give a few examples I noticed so far:
> 
> 1. In a lot of places GooString is dynamically allocated even though
> it could be placed on stack, which could save a malloc() a delete and
> reduce runtime memory usage.
> 
> 2. In a lot of places if (foo) gfree(foo) and if (foo) delete foo is
> used, even though just gfree(foo) and delete foo works just fine and
> is less code.
> 
> 3. Speed could probably be improved if SplashBitmap was restructured
> to avoid switch(mode) in every getPixel() call (e.g. by splitting into
> a base class and making each variant derive with virtual getPixel()).
> 
> 4. Based on my profiling, a speed could be improved a lot if
> lexing/parsing was restructured to not allocate so many temporary
> objects. But that would impact a lot of code.
> 
> So how do you decide whether to make a given change or not?

Basically, it's a trade off with no hard rules. Things to consider:
- How volatile the code is.
	- has it changed much in the past?
	- how 'complete' is it?
	- is it expected to change in the future?
- How beneficial the change is
- And probably most importantly, are you willing to take responsibility
  for keeping that part of the code at least as good or better than the
  corresponding xpdf code.

So for me, compiler warnings aren't that big of a deal, because they
don't provide any real benefits to the users of the library, they tend
to be spread widely across the code base instead of localized to a
specific area and, for me, being able to merge with xpdf is of more
value than not seeing the warnings. i.e. Derek does a better job
'maintaining' the code than I do, so if he wants the errors than that's
ok. Performance improvements, however, would be evaluated differently
because they do provide tangeable improvement for the users.

Hopefully, that gives a clearer picture of the way that I see it.

> How about I'll try to keep changes in master to non-invasive changes
> and do bigger improvements on a branch and you can decide if you want
> to merge them or not?

Sure, that's not a bad idea anyways. Plus, with git, using a branch is
not really different than using master (provided you can get 'push'
working right :))

-Jeff