Krita shared pointers usage

[Stefano Bonicatti]

> Well, I cannot agree with it. Passing a raw pointer down the stack and
acquiring it somewhere below will make the code crashy and really hard to
understand why. Who would guarantee how the > passed object will be used
inside the function? Will it be stored or not? How the caller has allocate
and stored the pointer? Does he own it or not? There might be really funny
situations, like:

I understand the issue you present and i think there's a difference on what
"we want" on a codebase:
I normally want to be in control of the codebase and know it very well so
if something like that happens i would know why it happened.

Don't want to go offtopic but "the developer shouldn't care" it's more
fitted for a library, but library are quite hard to make, so that the
developer really doesn't care how the underlying code works (and still some
information is given by the doc).
So what i mean is that i "never" trust, unless it's a well tested library,
the functions i call etc and check what they actually do, so that i can
also use them properly in other cases.

Again i understand the different point of view you may have, and respect
it, but hiding too much, while it can help with information overhead, might
also make actually harder to write code, imho.

Though when i said "pass down the stack a raw pointer", i meant in the case
you already had a shared pointer holding it (like in the rootLayer()
function in KisImage).
In the issue you present that memory should've been acquired by a shared
pointer, especially since it's a class (this should actually be a must) and
manual deletion should be banned for those types.

In general i think the same as him ->
http://www.stroustrup.com/C++11FAQ.html#std-shared_ptr, not only because he
his who he his, but because it actually make sense what he says about
making the object lifetime cycle complex when shared pointer are used so
often.

Maybe having unique_ptr like pointers and shared pointer passed as const
references instead of raw pointer would be better, so that it's more
explicit the fact that you should not delete it manually and it's cheaper,
copy wise and with the ref/deref not happening when passed around in code
that shouldn't actually prolong its life.
You would still have shared pointers around but it would be much easier for
the memory leak tracker to keep track on who is really holding the memory.

> The idea provided by Qt's shared pointers might be very helpful here
(Qt's weak shared pointer doesn't allow dereferencing the pointer until
upgrade to a real shared pointer)

There are a couple of problems with Krita weak pointers i'm facing though:
1) If you pass a dead weak pointer to a shared pointer the check to see if
the weak pointer is valid is done in an assert only after the shared
pointers tries to increase the reference count.
This works if the weak pointer internal pointer was set to NULL, because
shared pointer ref() function checks for that, but if the weak pointer
points to deleted memory.. well ref() behaviour is undefined.

2) Here i'm talking about that issue with the resource manager holding a
shared pointer to the current activated node, that should be deleted when
the image is deleted.
You suggested to use a weak pointer but due to how QVariant works, as of
now, you can't safely store (or actually retrieve) a weak pointer inside
because there's no way to check isValid() function before a copy happens
(through QVariant value<>() function), and this as point 1, may access to
deleted memory.
This is kind of complicated because you can put the isValid check in the
copy constructor before loading the pointer into the new weak shared
pointer, so that the internal pointer is set to 0 if it's not valid, but it
would add the overhead of the isValid check for a specific case to every
copy done.

This is why i think sometimes smart pointers get a bit in the way, they
make you think everything is safer while.. it depends..
For point 2 i think then it should be who actually set that node
(KoCanvasResourceProvider through an event), that always through an event
(image closed/deletion?) removes it (so the QVariant too), instead of
relying on a weak pointer?

> So as a conclusion for my long mail I would suggest to consider the
following:

Don't worry, as you can see mine is longer, obviously :-).

> 1) Use weak shared pointers in UI dockers or elements.
> 2) Upgrade weak shared pointer to real pointer before use in UI (?)
> 3) Don't use manual deletion of Private classes. Use
QScopedPointer<Private> instead. This is the most popular cause of memory
leaks and dangling shared pointer in Krita, btw. It also guarantees the
"c++ rule of three" is satisfied properly (it disables default copy-ctor by
default).

I agree with these, but can you elaborate more point 2 (what you mean with
real pointer?).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20141220/58a7485d/attachment.html>