Re: [GNC-dev] Preferred way of writing object oriented code

> On Mar 14, 2023, at 12:24 PM, Maarten Bosmans <[email protected]> wrote:
> 
> Hi,
> 
> In preparation for making some changes to QuickFill.c, I thought it
> would be a good idea to get familiar with the GnuCash code by doing
> some easy fixes or code improvements.
> So I turned on the AddressSanitizer of GCC and ran the test suite. The
> first issue I looked at was a delete of a GncGuid that was allocated
> with g_malloc(). Trying to untangle that led me through several
> parallel implementations of objects and containers: GLib, Qof, and
> C++. Let's just say that the codebase has a rich and varied history.
> 
> Of course this is not new to any current developers of GnuCash. On C++
> page on the wiki it is mentioned that C++ should be preferred over
> GLib and Qof. I think that makes sense.
> I have a couple of questions for further clarification, as I want to
> avoid spending time on preparing patches that you don't want to
> accept.
> - I see that GncGuid is implemented using boost::uuid now. But the
> implementation still interfaces with glib and qof. Is eliminating that
> seen as a worthwhile further improvement? (even if it is just
> replacing `gconstpointer` with `const *void`)
> - Could in general any GList instance that is not needed for
> interfacing with Gtk potentially be converted to the appropriate C++
> container?
> - Does this also mean that any .c file can be converted to .cpp if we
> need to make use of a C++ feature? (e.g. std::vector, or templated
> functions)


Rich and varied indeed!

As for GUID: It still has Glib and QOF interfaces because it's pervasive throughout the code-base. The way towards changing that is getting rid of Glib and QOF, not fixing them to use GUID instead of GncGUID.

Spelling changes like gconstpointer -> const void* (or void const* if you're an east-const person) isn't really useful on their own, but if you're doing something serious and that happens to be there then it's OK. Good refactorings of 400-line functions is a lot more useful if you want to clean up code. Writing unit tests is even better. 

Changing a GList to std::vector is always a nice improvement, but if it's an rv or out-variable of a public function then you have to make sure that all of the consumers are also compiled with C++, *including SWIG bindings*. That's a general statement for c -> c++ conversion of any file: You have to make sure that either all of the consumers of what that file exports in its public header are also compiled with C++ (in which case you should rename the header to .hpp to make it clear that it's a C++-only header) or that the exported functions are marked extern "C" and will compile in C. 

Does that help?

Regards,
John Ralls