gnc_commodity_table_get_namespaces returns a vector #1910
Conversation
christopherlam
force-pushed
the
progress
branch
4 times, most recently
from
23569ae
to
6ff411f
Compare
christopherlam
force-pushed
the
progress
branch
from
6ff411f
to
f5601c9
Compare
libgnucash/engine/gnc-commodity.cpp
Outdated
| g_hash_table_destroy(ns->cm_table); | ||
| std::for_each (ns->cm_table.begin(), ns->cm_table.end(), | ||
| [](auto it){ gnc_commodity_destroy (it.second); }); | ||
| StrCommodityMap().swap(ns->cm_table); |
There was a problem hiding this comment.
Sigh. Not your fault but this function should call only
{
gnc_commodity_namespace *ns = gnc_commodity_table_find_namespace(table, name_space);
g_object_unref(ns);
}
The cleanup stuff--removing ns from table->ns_table and table->ns_list and clearing the StrCommodityMap should be in gnc_commodity_namespace_dispose_real(). Call ns->cm_table.~StrCommodityMap() in gnc_commodity_namespace_finalize_real(). If you use placement new for constructing the StrCommodityMap then you can call Edit No, I remember now that you still have to use delete ns->cm_table instead, which is all-around cleaner.~StrCommodityMap() with placement new.
There's no point to having GObject memory management if we don't bleeping use it.
Better still of course would be to rewrite gnc_commodity_namespace as a proper GncCommodityNamespace C++ class, but that's probably more work than you want to do at this point.
libgnucash/engine/gnc-commodity.cpp
Outdated
| @@ -1592,7 +1593,7 @@ gnc_commodity_table * | |||
| gnc_commodity_table_new(void) | |||
| { | |||
| gnc_commodity_table * retval = g_new0(gnc_commodity_table, 1); | |||
| retval->ns_table = g_hash_table_new(&g_str_hash, &g_str_equal); | |||
| StrCommodityNSMap().swap (retval->ns_table); | |||
There was a problem hiding this comment.
As for StrCommodityMap, better to use placement new.
There was a problem hiding this comment.
As for StrCommodityMap, better to use placement new.
or auto retval = new gnc_commodity_table; which will properly initialize the map.
There was a problem hiding this comment.
It will, but you'll need an explicit ~gnc_commodity_table() to clean up the GList. Note that mixing new with malloc like that is considered risky. The compiler will probably do what you want but it doesn't have to.
|
This branch getting rather crowded. I wonder if it would be safer to push well-tested commits one by one. The aim is still to use c++ classes. |
christopherlam
force-pushed
the
progress
branch
3 times, most recently
from
b74d21e
to
74b1ea8
Compare
|
last commit is fake... don't know how to fix python bindings. |
gnc_commodity_table_get_namespacescurrently returns aGList*which isn't always released. There's more leaks similar to 4f5ee5d. Instead of hunting down leaks, forward-proof it to return astd::vector.Should it return astd::vector<std::string>?Dittopostponed to later branchgnc_commodity_table_get_commoditiesisn't always being released properly.