[poppler] Question on policy w.r.t. code modernization

[Adrián Pérez de Castro]

Hello all,

Quoting Albert Astals Cid (2015-12-28 19:55:44)
> El Friday 25 December 2015, a les 14:38:12, Adam Reichold va escriure:
> > Hello,
> > 
> > remembering how I tried to push some minor optimizations to GooString
> > some time ago, I wanted to ask about the project's policy on how to
> > handle code modernizations in the future.
> > 
> > To me, it seems like stuff that is pervasive like GooList or GooString
> > is unlikely to be changed with the currently available man power due to
> > the risk of regressions. One way of resolving this is IMHO to try to
> > remove the code completely and replace it with externally maintained and
> > highly optimized components, e.g. using std::vector and std::string from
> > the standard library instead of GooList and GooString. But this will
> > probably make it rather hard to merge xpdf changes in the future. Then
> > again, this seems to be hardly happening as is.
> 
> Do we actually get any benefit for using std::string instead of GooString?
> I can only see it creating regressions and lots of work for what is probably a 
> very marginal speed improvement (happy to be proven wrong) and also the class 
> does not give us any maintainance overhead since it's "done".
> 
> If we're going to diverge for the sake of modernizing the source code i think 
> that makes more sense adding overrides to overriden functions, adding const to 
> const functions and making sure that classes that should not be copied/
> assigned by value have the copy constructor and assignment operators deleted.

When I was implementing Tagged-PDF support, my intention was to make the new
parts of the API const-correct, but in some cases I had to leave some “const”
modifiers out because then constness would carry over to many other places
in the Poppler code. It is a task a little bigger than it may initially look
like, but it is definitely doable.

Also, for Tagged-PDF support I got to use std::{vector,set} internally because
it was not expected to merge patches from xpdf which would touch the newly
added source files. It was much more pleasant than using Goo{List,Hash}, and
the type safety provided by the std:: containers was certainly an aid. I would
like to see those used more, but I also think it shouldn't be made “just
because”: either there's an improvement, or otherwise probably it's better to
leave code that works as-is.

(For the record, I would count “less memory used” as an improvement. Removing
utility code from and using the C++ standard library will likely reduce memory
footprint in the long term — to begin with, there wouldn't be more than one
implementation of list/hash/string/etc, and that will immediately marginally
less memory used for the code segment of libpoppler.)

Another thing that comes to mind that would be interesting to use is RAII.
Very often in Poppler code there's temporary objects which need to be
“delete”d after using them. Ideally, those should know how to deallocate
themselves automatically when they go out of scope. Related to that would be
using smart pointers (shared_ptr<>/unique_ptr<>) for heap-allocated objects.

What do the rest of you think about RAII/smartpointers, and trying to migrate
to std:: containers progressively?

Best Regards,

--
 ⌨ Adrian

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: signature
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20151229/312cf415/attachment.sig>