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

[Albert Astals Cid]

El Tuesday 29 December 2015, a les 17:02:50, Adrián Pérez de Castro va 
escriure:
> 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?

Object is the one that would probably most benefit from at least autodeleters 
when it goes out of scope, but has to be done carefully since it is sometimes 
copied, sometimes ref'ed, but yeah there's lots of places that the management 
of free() on the Object instances is hard, but then again lots of places the 
same Object is freed and then reused later, so it would need careful thinking.

FWIW i introduced something like an autodeleter for the GooMutex as the 
MutexLocker class.

Also there's the question if having the same thing done in two different ways 
(i.e. partial migration) helps with the code redibility or hinders it, 
undecided myself.

Cheers,
  Albert

> 
> Best Regards,
> 
> --
>  ⌨ Adrian