On 02.08.21 22:34, Will Godfrey wrote:
I won't start rc1 until you're both happy with things generally.
Am 05.08.21 um 01:42 schrieb Ichthyostega:
... I am on a travel within Germany at the moment. But I have taken my
Laptop along, and I will do a review when I find some time next days.
Hi Will,
Hi Kristian,
today I travelled by train back to Munich, and so I had ample time
to look at the indicated changes. Basically I followed all usages
of the MasterUI::filerlist and the class FilerLine.
While I am rather not familiar with details of FLTK, the usage of
the Vector and how you clear it before reading in a new directory
or when closing the UI all looks correct for me.
At that point, the next question is: how much do we want to go
down the rabbit hole? Because, on second thought, what we do here
now is a bit silly (while correct as it stands): The whole point
with those STL containers is that they provide an airtight and
convenient automatic memory management. However, we use that
automatically managed heap memory just to store pointers,
which we then manage manually as if it was just an array.
Thus we could consider to abandon using those pointers and
the explicit delete calls; rather we could just place our
payload directly into the heap memory managed by the container
and leave all the hard work to the compiler.
However this might lead into a pitfall: since, after populating
this list, we add the created UI elements (the FilterLine instances)
to some other UI widget (the filerscroll), and we do so by pointer.
Which means, those other parts of the UI rely on those pointers
to remain stable. Now, the way we populate and use this filerlist,
those pointers will indeed remain stable -- but, if some time into
the future someone adds code to manipulate the contents of the filerlist
further, this might cause the STL container to re-allocate and then the
old pointers won't be valid anymore.
Now, the standard / idiomatic C++ way to protect against problems of
this kind is to make the payload class "non-copyable". We can achieve this
by declaring the copy constructors and assignment operators as "deleted".
When we do this, the compiler immediately gives us an error, simply
because std::vector is implemented such as to re-allocate its contents
on growth. And thus std::vector needs contained objects to be at least
"movable" (i.e. they need at least a move-constructor). But there are
other STL containers, which are able to handle non-copyable objects,
most notably the std::list and the std::deque. In this case, the latter
will perform way better, since it allocates the memory in small chunks
of typically 8 elements at once, and then chains those. Thus a Deque
is a good compromise between a vector and a double-linked list. To
quote from the C++ reference:
https://en.cppreference.com/w/cpp/container/deque
The storage of a deque is automatically expanded and contracted as needed.
Expansion of a deque is cheaper than the expansion of a std::vector because
it does not involve copying of the existing elements to a new memory
location. On the other hand, deques typically have large minimal memory
cost; a deque holding just one element has to allocate its full internal
array at least once...
With these adjustments, we can then "emplace" the FilerLine,
i.e. we create it in-place within the container.
You might want to have a look at those changes proposed hereby in my
Gitub-Repository: https://github.com/Ichthyostega/yoshimi.git
Branch: "review"
https://github.com/Yoshimi/yoshimi/compare/master...Ichthyostega:review
I compiled with those changes and then tried to open a new root or
load an external Instrument (those would use the filer, correct?)
and it seems to work as expected.
-- Hermann
_______________________________________________
Yoshimi-devel mailing list
Yoshimi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
