[poppler] Changes to 'better_object'

[Albert Astals Cid]

El dissabte, 6 de maig de 2017, a les 9:44:44 CEST, Carlos Garcia Campos va 
escriure:
> Albert Astals Cid <aacid at kemper.freedesktop.org> writes:
> > New branch 'better_object' available with the following commits:
> > commit 3c29ded4bee5eadb829ed46af2ec92be57b0077b
> > Author: Albert Astals Cid <aacid at kde.org>
> > Date:   Fri May 5 23:32:32 2017 +0200
> > 
> >     Make Object free itself on init and destruction
> >     
> >     Will make for a *much* easier way to code.
> 
> Great \o/ I've taken a quick look at it and I have a few questions.
> 
>  - Do we really want to have a dead type for Objects after a move? We
>    can make moved objects reusable by simply initializing them to
>    None. 

They're still reusable if you call one of the initBla() functions, but it's 
nice to have the dead state so that if you try to call a getBla() function it 
tells me i did some mistake in the porting.

>    I'm not a C++ expert, but I think the state of a moved object
>    is actually undefined, but that doesn't mean you can't make it
>    defined in your objects. For example std::unique_ptr has a defined
>    behavior, after a move the pointer is initialized to nullptr, like if
>    it had just been created and you can reuse a moved unique_ptr by
>    using reset().
> 
>  - Why keeping copy and shallowCopy? Can't we make copy be the =
>    operator and shallowCopy the move?

shallowCopy will go away in the next patch, not sure about copy, probably can 
make it go away too.

> 
>  - The change in Annot.cc is weird
> 
>    Dict *dict = new Dict(xref);
> +  dict->decRef();
>    dict->add(copyString("Font"), &fontsDictObj);
> 
> why do you need to unref an object you have just created?

That was a bug in the existing code, because Object::initDict increases the 
refcount on the Dict and thus we had a leak, Object::initDict must die, i have 
a todo for it :D

> 
> >     Patches with more std::move coming on top.
> 
> Great! I think the other common mistake I've seen in poppler more often
> is uninitialized class members. It's easy to forget to initialize
> everything in all the constructors, so I think it would also be useful
> to move all the initializations to the headers in the class
> declarations. So for example
> 
> Foo:Foo()
> 
>   : ptr(nullptr)
> 
>   , n_foo(0)
> {
> }
> 
> becames
> 
> class Foo {
> private:
>   void* ptr { nullptr };
>   unsigned n_foo { 0 };
> };
> 
> What do you think?

Personally i dislike it, but i guess it's a matter of getting used to it :D

Anyway on a separate patchset/branch :)

Cheers,
  Albert

> 
> >     Most things seem to work though i'm pretty sure some things are
> >     broken.
> >     
> >     NEEDS TESTING
> > 
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/poppler