On 08.08.2021 05:35, Rainer Hans Liffers wrote: > Hi Will, Hermann, and Kristian > > On 8/8/21 9:35 am, Ichthyostega wrote: >> >> 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. > I am not familiar with the code details, but the statement above > seems to imply that FilterLine instances are /shared/ by a list of such > instances and various filerscroll elements. If that is the case, then > IMHO the list should accordingly contain shared elements, ie it > should eg be of type std::vector <std::shared_ptr <FilerLine>>, and > the various filerscroll instances should contain elements of type > std::shared_ptr <FilerLine>. Sharing pointers directly is always a > bad idea.
I agree with Rainer, I think that switching to shared_ptr is far less intrusive than the other suggested approaches, and is still both idiomatic and safe. -- Kristian >> 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 >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/yoshimi-devel > > > > _______________________________________________ > Yoshimi-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/yoshimi-devel > _______________________________________________ Yoshimi-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
