GNOME Bugzilla – Bug 736687
Kvp C++ Restructuring to use boost::variant, std::vector, etc.
Last modified: 2018-06-29 23:33:45 UTC
We have some goals for the KVP: * Rewrite using c++ for easier maintainability and some possible speed gains * Make it invisible to client code, i.e. an implementation detail.
Created attachment 286224 [details] [review] KvpValue restructuring 1.
Attached is a patch that rewrites kvpvalue using `boost::variant`. It doesn't touch `kvp_frame`. Currently `kvp_frame.h` says <code>typedef struct _KvpValue KvpValue;</code> (I hope this turns out well...I don't see a way to preview my markup!) I was hoping to leave this as is, and do <code>typedef boost::variant<...> _KvpValue;</code> so that none of the headers need to change. A KvpValue * would be an opaque pointer to the outside world, and would be a type-checked pointer to `boost::variant` within the `.cpp` file. This didn't work because the first typedef sayas that `_KvpValue` is going to be a struct, *not* a typedef to a struct! My next attempt was <code>struct _KvpValue : public boost::variant<...> { using variant::variant; }</code> And this almost worked. It looks like variant isn't designed to be extended. The result of this approach is that the compiler wasn't able to help as much as it would be desired. There needed to be a lot of casts to get to the objects within the variant, etc. I decided the best approach would be what is attached. The header has <code>typedef void * KvpValueP;</code> So that outside, this is referred to as KvpValueP (outside code should never be dereferencing this object), an opaque pointer. Then, within the `.cpp`, we have <code>typedef variant<...> kvpvalue;</code> And a reinterpret_cast from KvpValueP to kvpvalue* is required any time you want to use it as a `boost::variant`. This is roughly the same as the work on GUID except that GUID was a POD, so it could be defined to C code as char[...]. This is not a good way to go with `boost::variant` because it's much more complicated.
Comment on attachment 286224 [details] [review] KvpValue restructuring 1. 1. Don't inflate the patch with extraneous editorial changes. A commit should do only one thing. 2. That aside, don't alias pointers to make them look like plain types. That's very bad C++ style. 3. Use C++11 style: 'using' instead of 'typedef'. 4. Don't 'use namespace foo'. You can shorten a namespace if it's really long (e.g. 'namespace mp = boost::multiprecision';) but 'use namespace' defeats the whole point of having namespaces. 4. Separate the C++ implementation from the C wrappers. We want two interfaces so that new C++ doesn't have to go through C functions, and hide the implementation so that it can be changed later without affecting the outside world. That means you should have a KvpValueImpl class with accessors that hide the mechanics of using boost::variant, and C accessors that just call the KvpValueImpl member function.
Thanks for the feedback! 1. I'm not sure what you mean by editorial changes. I thought about breaking up the commit somehow, but two things slowed me down: 1) I don't think I could break it up and still have it compile, and 2) even if I could, I wasn't sure if you would like to see multiple commits each doing "one thing" if they didn't work independently of one another. 2. Yup, sounds good. 3. Yup, sounds good. 4.1 Sure thing. 4.2 This is an excellent suggestion, and will shape my future efforts.
By "editorial" I mean changes that don't do anything: Fixing whitespace, adding comments without changing code, replacing NULL with nullptr (BTW I'm not sure that's a good idea in C-linkage code), etc.. In this case, changing KvpValue* to KvpValueP seems to be the only change in most of the files you touched. That should be a separate commit, except of course that it's a bad idea altogether because C++ has three synonyms with somewhat different nuances (KvpValue*, KvpValue&, and KvpValue&&). Hiding one of them behind an alias makes it less clear what you're doing. Two more comments: * Use stdlib containers inside the C++ implementation, always preferring std::vector as the default unless there are very good reasons to use something else.(Scott Meyers, "Effective STL", Addison-Wesley, 2001, p12.). * Don't bring GLib features into the C++ implementation unless you absolutely must. The KvpValue GList* actually stores a list of KvpValue*; that can be a std::vector<KvpValueImpl> instead.
Okay, thanks. I agree that it should be (and it can be) a separate commit. The plan is for KvpValueP only for C code. C++ code should only use KvpValue (and explicit references and pointers). The problem with using std::vector<KvpValue *> is that C code iterates the list, etc. (at least in the tests). We could put that behind an interface, too, adding a function like get_value(KvpValueP, unsigned int whichone); and that would allow us to hide the implementation. What do you think?
Sorry, I don't see any point to changing KvpValue* to KvpValueP in the C code, either. Can you explain your rationale, considering especially that any C code that directly accesses KVP is high on the list for deletion as soon as it's feasible? Yes, of course iterating over the std::vector<KvpValueImpl> should be hidden behind the interface. I think that kvp_value_get_glist() can just create a GList on the fly by iterating over the array, something like kvp_value_get_glist (const KvpValue* value) { GList* list; std::vector <KvpValueImpl>& v = KvpValueImpl (value).getVector(); std::for_each(v.rbegin (), v.rend (), [](auto elem){list = g_list_prepend (list, &elem);}); return list; } That might take some tweaking to get it to work exactly right, and you'll need to have some sort of error handling in there for cases where value doesn't point at something that has a vector. It does occur to me that while in general it's better to put actual values in the vector as long as they're not too big, in this case it might be better to have pointers because of the potential for nesting. You'll have to evaluate the usage in GC to see what works least badly.
Created attachment 286324 [details] [review] This patch removes the binary option from kvpvalues. Attached is a patch that eliminates binary from kvp value types. If I should open a new bug for this, please let me know. Your comments on GList sound just fine. I'll take a look at the uses of GList from KvpValue. I'm wondering if it's not actually used in the codebase, and only in the test? (crosses fingers)
(In reply to comment #8) > Your comments on GList sound just fine. I'll take a look at the uses of GList > from KvpValue. I'm wondering if it's not actually used in the codebase, and > only in the test? (crosses fingers) Wasn't this the source of the problem reported by Christan Stimming recently in online banking ? I seem to remember him explaining online banking stores a glist of kvp's...
(In reply to comment #9) > (In reply to comment #8) > > Your comments on GList sound just fine. I'll take a look at the uses of GList > > from KvpValue. I'm wondering if it's not actually used in the codebase, and > > only in the test? (crosses fingers) > > Wasn't this the source of the problem reported by Christan Stimming recently in > online banking ? I seem to remember him explaining online banking stores a > glist of kvp's... Yeah, unfortunately GLists of KVP are used in a couple of places. In addition to the hbci-templates list Christian brought up on the dev list there's also gnc_kvp_bag_* stuff used by Scrub2. The good news is that it doesn't look like there's any nested lists, so the implementation I sketched out above should be OK.
Comment on attachment 286324 [details] [review] This patch removes the binary option from kvpvalues. No need for a new bug report. Pushed to master. Thanks!
There is another issue with replacing the GLists with an hidden implementation: KvpValues are also accessed through gvalues through qof_instance_get_kvp. This is used some 40 times. I'm not sure how to handle this type of thing... I'll start takinga look, perhaps all the calls to qof_instance_get_kvp can return KvpValue* rather than gvalue* ?
(In reply to comment #12) > There is another issue with replacing the GLists with an hidden implementation: > KvpValues are also accessed through gvalues through qof_instance_get_kvp. This > is used some 40 times. I'm not sure how to handle this type of thing... I'll > start takinga look, perhaps all the calls to qof_instance_get_kvp can return > KvpValue* rather than gvalue* ? KvpValues are translated to and from GValues by gvalue_from_kvp_value() and kvp_value_from_gvalue() both in kvp_frame.cpp. They use helper functions for handling lists. The short answer to your issue is to modify those functions as needed to handle the new internal representations. The longer answer requires a bit of digression: qof_instance_get_kvp() is another matter entirely. It returns the raw frame for the caller to modify directly. That's gonna break, which is good because it's evil. The only reason we're using GValues is for the g_object_get() and g_object_set() (which are wrapped with qof_instance_get and _set to ensure that our own housekeeping gets done). That mechanism is obviously going away. It exists only to work around certain deficiencies that GObject has (like no way to parameterize constructors). KVP itself originated as a dodge to allow adding fields to the database (or elements to the XML file) without older versions of GnuCash noticing. This allows a certain amount of flexibility as long as KVP data are important only for some feature in the newer version that doesn't interact with what the older version does. My objection to it is that it was widely used by getting a pointer to an object's KVP and putting stuff in it without the object knowing about it. That caused all sorts of data integrity issues for the object because it didn't know it had changes to commit. Most of the time KVP was just used to add some fields to classes, and for those cases we can just make them member variables of the class; only the backend needs to know that it's stored as KVP instead of regular fields/elements. There are some other cases where KVP was used to create whole new features. Some that I know about are import account matching, AQBanking setup, and business info for invoicing and such. Getting control of those is a bit more complicated because a single instance of e.g. Account might have thousands of import-matching records associated with it.
Created attachment 286436 [details] [review] Apply to gnucash/master. Removes a deprecated #define There is a #define that's marked deprecated: #define kvp_value KvpValue in kvp_frame.h that an attached patch removes.
Comment on attachment 286436 [details] [review] Apply to gnucash/master. Removes a deprecated #define Pushed to master; the patch wouldn't apply to maint because some of the instances have been moved. This still might be worth doing in maint, but there will be some merge-back issues. Are you doing one for KvpFrame too?
(In reply to comment #15) > (From update of attachment 286436 [details] [review]) > Pushed to master; the patch wouldn't apply to maint because some of the > instances have been moved. This still might be worth doing in maint, but there > will be some merge-back issues. This patch breaks make distcheck on master for me. I started from a clean environment and ran into this: /kobaltnet/janssege/Development/EclipseGnuCash/GnuCash-git/src/doc/design/gnucash-design.texi:136: @detailmenu reference to nonexistent node `kvp_value' mv: cannot move ‘.am3863/gnucash-design.info’ to ‘.//kobaltnet/janssege/Development/EclipseGnuCash/GnuCash-git/src/doc/design/’: No such file or directory make[5]: *** [/kobaltnet/janssege/Development/EclipseGnuCash/GnuCash-git/src/doc/design/gnucash-design.info] Error 1 make[5]: Leaving directory `/kobaltnet/janssege/Development/Builds/gnucash-f20-master/src/doc/design' make[4]: *** [distdir] Error 2 make[4]: Leaving directory `/kobaltnet/janssege/Development/Builds/gnucash-f20-master/src/doc/design' make[3]: *** [distdir] Error 1 make[3]: Leaving directory `/kobaltnet/janssege/Development/Builds/gnucash-f20-master/src/doc' make[2]: *** [distdir] Error 1 make[2]: Leaving directory `/kobaltnet/janssege/Development/Builds/gnucash-f20-master/src' make[1]: *** [distdir] Error 1 make[1]: Leaving directory `/kobaltnet/janssege/Development/Builds/gnucash-f20-master' make: *** [dist] Error 2 Looks like some references to kvp_value have been missed in the texi files.
Fixed. Bear in mind that the whole src/doc directory can be deleted once Carsten gets it moved to the wiki, and also that there's no reason to run distcheck on master until spring of 2016 when we start the 2.7 releases.
(In reply to comment #17) > Fixed. > > Bear in mind that the whole src/doc directory can be deleted once Carsten gets > it moved to the wiki, and also that there's no reason to run distcheck on > master until spring of 2016 when we start the 2.7 releases. I don't particularly agree that there is no reason to run distcheck on master until Spring 2016. I think it's very healthy to make sure the code passes check (and distcheck) along the way. It means we *could* theoretically branch and make a release without having to track down all the potential failures of distcheck. It also means you have many fewer changes to search through to figure out why distcheck is failing. Arguably the nightly build should run a distcheck, although I know it doesn't work on Windows. Alas, we don't have a Linux daily builder. Maybe we should set one up?
We're wandering away from the bug topic; perhaps we should continue this on the dev list... A Linux daily builder with failure reports to the dev-list would be great. A better answer than running distcheck routinely is making sure that check actually checks everything and that make actually makes everything. IIRC only make distcheck makes the info files and only check the translations. That's bad, both should be done by make. The only new errors distcheck should be capable of surfacing are files not making it into the tarball or not getting cleaned, both of which can be very quickly fixed at release time. As for check, yes, make check should pass for every commit before it's pushed. The hard part is that ideally it should be run on several Linux and BSD distros, several versions of OSX, and on Windows, both 32- and 64-bit where applicable. I'll do multiple builds, though not quite that many, before pushing a big change, but it takes to much time to do it for every bug-fix.
"Are you doing one for KvpFrame too?" Some day. Right now, I'm not touching KvpFrame yet, only KvpValue. I have written up a KvpValueImpl class a while back, but make check isn't passing. First, I had a bit to learn about constructor templates, then it failed because I was returning {} for the case when a gnc_numeric was retrieved from a null kvp_value (rather than gnc_numeric_zero()). Now, I'm not sure what the problem is. After I get everything ironed out, I'll put the patch here for kvp_value. Then, either I'll run away to qof instance or some backend thing, or I'll head over to kvp_frame. I am, as always, open to suggestions.
I only meant the deprecated '#define kvp_frame KvpFrame' one line above the kvp_value macro that your patch nukes. Since it's a simple sed fix I thought it natural to just do it. If you're not inclined I'll do it next week.
Right. I do plan to take care of that after I'm happy with kvp_value, but it would be fine if you do that instead.
Created attachment 287399 [details] [review] Implement KvpValue in c++ using boost::variant Attached is a patch that deals with KvpValue. I had to deal with a few spots where we expect something valuable to happen when querying a null pointer.
Comment on attachment 287399 [details] [review] Implement KvpValue in c++ using boost::variant Doesn't compile with Clang: /Users/john/Development/Gnucash-Build/Gnucash-master-git/inst/include/boost/variant/variant.hpp:247:26: error: no template named 'is_nothrow_move_assignable' in namespace 'boost'; did you mean 'std::is_nothrow_move_assignable'? Types, mpl::not_<boost::is_nothrow_move_assignable<boost::mpl::_1> > ^~~~~~~ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/type_traits:2796:51: note: 'std::is_nothrow_move_assignable' declared here template <class _Tp> struct _LIBCPP_TYPE_VIS_ONLY is_nothrow_move_assignable ^ /Users/john/Development/gtk-sources/gnucash-git/src/libqof/qof/kvp_frame.cpp:119:9: error: use of undeclared identifier 'isnan' if (isnan(one) && isnan(two)) ^ /Users/john/Development/gtk-sources/gnucash-git/src/libqof/qof/kvp_frame.cpp:119:23: error: use of undeclared identifier 'isnan' if (isnan(one) && isnan(two)) Is this just a first step? If not, what does it do for us? Why reinterpret_cast instead of static_cast? I don't think throwing const char* is something we should do. Use one of the standard exceptions.
I don't understand the error. What version of clang are you using? I used version 3.5-1ubuntu1, was able to compile, but I wasn't even able to make check gnucash/master (7a22644). I currently do make && make check; before I put any commits here, are there more steps I should do? It is a first step. This version of kvp_value exposes its functionality in a way that can be optimized (compiler inlining, and hopefully less run-time type computations through the use of templates and boost visitors), and has better type-checking. Also, it will be relatively easy to change the GList* usage within kvpvalue to std::vector (for instance). isnan should be std::isnan. I chose reinterpret_cast because I want the hand-off-ness thereof. I don't want any conversions to be attempted, etc., I just want the pointer to be reinterpreted. Would you prefer static_cast? Yup, I'll pick a standard exception instead of char const *.
Apple LLVM version 6.0 (clang-600.0.51) (based on LLVM 3.5svn) Actually, it's not std::isnan, just isnan. The fix was a couple of extra includes, <boost/type_traits/is_nothrow_move_assignable.hpp> and <math.h>. The former is a bug in the version of boost I'm using (1.56.0): boost/variant/variant.hpp includes boost/type_traits/is_nothrow_move_constructable.hpp but not is_nothrow_move_assignable. I bet you're using an older version of boost and variant/variant.hpp doesn't have struct is_variant_move_noexcept_assignable. In general reinterpret_cast should be avoided and deserves a comment explaining why it's necessary. Static_cast is slightly less dangerous, and since we're getting a void* coming in we're stuck with one or the other, so use static_cast unless the compiler complains.
My boost claims to be boost_1_56_0. I installed from source. boost1.65.0/boost/variant/variant.hpp defines a struct is_variant_move_noexcept_constructible;... oh well. No problem, static_cast it is. I have also separated out kvp_value.cpp. I realized this was necessary when I started working on KvpFrame. In fact, should we do kvp_value_impl.cpp for the definition of struct KvpValueImpl; ? Or perhaps even KvpValueImpl.{c,h}pp? Having the C api, the KvpValueImpl, and the KvpFrameImpl implementations all in kvp_frame.cpp doesn't feel right.
Sorry, I have a typo in there. It should be 56 all round. 56.
Let's just have kvp-value.cpp/hpp. Note the -, not _; the _ in kvp_frame was a mistake when the file was created long ago. Almost all other files are named with -. I know that Christian thinks we should have a file-per-class, but Impl classes are by definition private; the API is for KvpValue. I've also been bitten more than once over in Gtk-land when someone created a file with the same name except for capitalization; it blows up on a mac because the file system treats them as the same file name. IMO that supports Derek's argument for all-lower-case filenames.
Created attachment 287545 [details] [review] kvp-value.{c,h}pp with static_cast, etc. Attached is a new patch. Let me know if it gives any compilation errors, etc., or if there are any other stylistic changes I should make.
The include of is_nothrow_move_assignable.hpp needs to be *before* including variant.hpp otherwise it has no effect. If you include variant.hpp in kvp-frame.hpp you don't need to--indeed shouldn't--include it in kvp-frame.cpp too. ERROR:/Users/john/Development/gtk-sources/gnucash-git/src/libqof/qof/test/test-kvp_frame.c:470:test_kvp_frame_add_frame_nc: assertion failed: (kvp_value_get_type( result_value ) == KVP_TYPE_FRAME) ... /qof/kvp_frame/kvp frame add frame nc: FAIL I don't have time to debug this right now, so if it passes for you it will have to wait until I do. So something's not quite right. A diagnostic print shows it's getting KVP_TYPE_INVALID. While the '#ifdef __cplusplus__' is superfluous because kvp_frame.cpp is always C++, the extern 'C' { } around the C-linkage headers isn't. Not all linkers are smart enough to not mangle the names of the symbols in those headers, and you can't count on C-linkage headers to have their own extern 'C' declaration. kvp-value.hpp and .cpp need the FSF boilerplate at the top. Why did you remove the const declarations from the function signatures in kvp_frame.h?
10-4 on the includes. I'm unable to reproduce the error you gave from test_kvp_frame_add_frame_nc, although I get a different error compiling with my clang version. I'm unable to look into it now, but I hopefully shall tomorrow. You're right, it's not a good idea to depend on C headers using extern "C". In practice, they usually do, but it's better to make it explicit. FSF boiler plate added. The const qualifier was removed from KvpValue*s in the set_slot* functions because g_hash_table_insert doesn't take a const pointer. Even if I cast away the const qualifier in insertion, it would still be a lie because kvp_frame_get_slot doesn't return a const pointer.
Created attachment 287701 [details] [review] kvp-value.{c,h}pp with some other adjustments False alarm on the clang errors, I cannot reproduce them. It had to do with older installed libraries. After uninstalling, then rebuilding from scratch, everything works just fine under clang and g++. I'll go ahead and update the patch according to current comments.
I've figured out the problem with Apple Clang: It's generating different typeinfos for KvpFrame*, one for when the underlying struct is known, the other where it isn't. I can't tell which is getting stored in KvpValueImpl::datastore::type, but moving the struct _KvpFrame definition into kvp_frame.h fixes the problem. The actual fix should be to move it into a new kvp_frame-p.h and include that in kvp-value.cpp and kvp_frame.cpp.
Small nitpick: wouldn't it be better to use the same hyphenation in all files describing the same object ? So use kvp-frame-p.h and kvp-frame.cpp instead of kvp_frame-p.h and kvp_frame.cpp ? Or is this a mixture of old and new api files ?
It's a mixture of old and new.
Created attachment 287924 [details] [review] kvp-value.{c,h}pp with some more adjustments Attached is the patch which differs from the previous patch by placing the definition of KvpFrame into its own header file that is included from kvp value and kvp frame implementations. Hopefully this will work around the issue we saw before. Also, noexcept wasn't being used consistently, and we switched to <cmath> std::isnan rather than <math.h> isnan. Naturally, it passes make check;
Created attachment 287982 [details] [review] Remove the deprecated kvp_frame typedef for KvpFrame. Attached is a patch that removes the deprecated typedef kvp_frame from KvpFrame.hpp (and all its uses).
Created attachment 287987 [details] [review] Remove the deprecated kvp_frame typedef for KvpFrame. The patch was incomplete. Running make clean from a clean build tree revealed a few more instances, and Mr. Ralls was able to point me to a few more that aren't compiled by make; or make check;.
Comment on attachment 287987 [details] [review] Remove the deprecated kvp_frame typedef for KvpFrame. With a couple of minor fixes in aqbanking.
Created attachment 288579 [details] [review] Implement KvpFrame Using std::vector Rather than GList Attached is a patch that adjusts KvpFrame away from GList toward std::vector<KvpValueImpl>. There were some API elements that exposed the GList directly; those have been replaced by an API to retrieve keys, which can then be used to iterate through the elements. The call sites have been updated. Let me know what you think!
Created attachment 288580 [details] [review] Implement KvpFrame Using std::vector Rather than GList I had forgotten to rebase against gnucash/master. I have done that and the resulting patch is attached.
Created attachment 289755 [details] [review] Fix kvp-value Delete Functionality kvp-value delete function wasn't correct. The template parameter was always a boost::variant<...> rather than getting the object from the boost::variant and passing it along. I remember discovering this a while back when doing the string conversion and comparisons, and I converted them to visitors. I'm not sure why I thought the delete function was working. The current patch creates a delete visitor which does properly delete the contents of the boost::variant. I think we could benefit from (if only at testing time) reference tracking as a matter of course, to catch errors like this. If no deletes ever happened, I think make check; would still succeed.
Comment on attachment 288580 [details] [review] Implement KvpFrame Using std::vector Rather than GList Generally looks good, but it fails make test: ERROR:/Users/john/Development/gtk-sources/gnucash-git/src/libqof/qof/test/test-kvp_frame.c:471:test_kvp_frame_add_frame_nc: assertion failed: (kvp_value_get_type( result_value ) == KVP_TYPE_FRAME) You forgot the copyright statement in kvp_frame.hpp. Please use all-caps for preprocessor macro names, and put the guard macro after the GPL block. Please also prefer 'class' to 'struct' for most cases, make data members private, and use m_foo for their names: IOW, private: map_type m_valuemap;
(In reply to comment #43) Pushed to master. > I think we could benefit from (if only at testing time) reference tracking as > a matter of course, to catch errors like this. If no deletes ever happened, I > think make check; would still succeed. What do you mean by "reference tracking"? Once everything is in C++ we can use std::shared_ptr for the stored object, but it presents the risk of crashing as long as we have to pass out raw pointers to C. The way to make 'make check' not succeed is to add tests for delete.
Created attachment 289925 [details] [review] Implement KvpFrame Using std::map Rather than GList As suggested, the patch has adjusted copyright notices, prefers class to struct, makes fields private, prefixes fields with 'm_', and contains any necessary refactoring to that end. I also made the adjustments to kvp-value as well. I was unable to reproduce the make check; error. I'm hoping that's just an artifact of where in the tree the patch was applied? This patch should be applied to 6c2a42b. Do let me know if anything goes awry. I'll plan to submit some google tests for kvp after a while, then I was planning to look at qof session if that sounds like a good idea.
Comment on attachment 289925 [details] [review] Implement KvpFrame Using std::map Rather than GList Pushed to master with a couple of minor changes.
Created attachment 290036 [details] [review] Adds KvpValueImpl test suite Attached is a patch that adds tests for KvpValueImpl as suggested before. I think we just one one executable artifact from make check; so I only use one .cpp file which includes the others. I believe I saw this approach in a gtest sample somewhere. Anyway, if a different structure is desired, please let me know, and I'll be happy to acquiesce.
Comment on attachment 290036 [details] [review] Adds KvpValueImpl test suite Test files need a GPL block with a copyright notice just like everything else. I don't think a single giant test program is at all desirable with gtest. Creating a separate program for each source file (or rarely class if one spans multiple files) helps to maintain the independence of classes. Try to make test programs stand-alone, with no external libraries other than libgtest.a and libgmock.a. Use mocks instead of actual external objects. Please look at https://github.com/jralls/gnucash/commit/ac126b1da415ab93752d8685697050dd876a4842 for a template on how to construct a test.
(In reply to comment #49) > Please look at > https://github.com/jralls/gnucash/commit/ac126b1da415ab93752d8685697050dd876a4842 > for a template on how to construct a test. Make that https://github.com/jralls/gnucash/commit/75815bb1835b6bc3d4885344ee91306caa838321. I force-pushed after rebasing on yesterday's test Makfile reallignments.
Created attachment 290558 [details] [review] kvp value impl c++ test suite Ah, I see. That might make for a bit of a noisy Makefile.am, but it doesn't make any difference to me.
Yikes, my patch is not very good! I didn't get the copyright notices, nor did I try to use any mocks! I'll try again, sorry for the delay.
Wow, I spaced that you'd even attached a patch. Sorry!
+/** + * KvpValueImpl should generally not be used on the stack because its + * destructor frees its contents, and it doesn't know anything + * about move semantics. Cases such as + * KvpValueImpl v1; + * auto guid = guid_new (); + * v1 = KvpValueImpl {guid}; + * fail because the third line deletes the guid in KvpValueImpl's destructor. + * Passing by value has similar problems. + */ That doesn't make any sense at all. First of all, KvpValueImpl doesn't have a default constructor so the first line won't even compile. If it did, it would be whatever was the default payload, not guid, that was deleted in the third line; the guid would be deleted when v1 goes out of scope. That's actually a feature, because otherwise guid would be leaked. It would be better if guid was on the stack, too, so the compiler could just move it around (I don't see anything in either class that would prevent the compiler from generating the implicit move constructors) or copy it. A heap allocation is way more expensive than copying a guid, so there's no good reason to allocate it.
Created attachment 290881 [details] [review] Create a kvp value impl test suite I added the copyright notice on top of the kvp value test suite, but I don't see how mocks can be useful in this particular test suite. Mocks are used to stuff dummy objects into the object that you're testing to make sure that the object being tested interacts with other objects in expected ways. An example would be: when testing a serialization class, provide a dummy "output stream" class and, with real input, make sure that the output into the dummy output stream is as expected. In this case, since kvpvalue is at the bottom of the dependency chain (it's more like an 'int' than a 'vector'), it doesn't interact with other classes much. One exception would be the glist stuff, but, since this isn't done in an object-oriented way, there isn't a way to mock glist interactions. Let me know if you disagree, otherwise, I think the attached patch should be good to go.
> That doesn't make any sense at all. First of all, KvpValueImpl doesn't have a > default constructor so the first line won't even compile. It's pseudo-code :-). Change it to KvpValueImpl v1 {1.1}; or KvpValueImpl v1 {(int64_t)5}; or whatever. That part doesn't matter. > If it did, it would > be whatever was the default payload, not guid, that was deleted in the third > line; the guid would be deleted when v1 goes out of scope. I agree that the first payload of v1 will be deleted, but the temporary created by KvpValueImpl {guid} is what shouldn't be deleted. Since KvpValueImpl doesn't know about move semantics, it isn't able to preserve the contents of the temporary KvpValueImpl correctly, and it assigns a deleted guid pointer to v1. Now, as I'm writing this, I find it somewhat hard to believe, but that's the meaning of the comment! I'll try to remember to take a look and verify the findings when I have time. > That's actually a > feature, because otherwise guid would be leaked. It would be better if guid was > on the stack, too, so the compiler could just move it around (I don't see > anything in either class that would prevent the compiler from generating the > implicit move constructors) or copy it. A heap allocation is way more expensive > than copying a guid, so there's no good reason to allocate it. A good deal of code depends on KvpValueImpl taking a pointer (to guid, or glist, or Kvp_Frame, etc.) and (deeply) deleting the contents of that pointer later. So the "good reason to allocate it" is because KvpValueImpl expects a 'guid_new'ed guid, and it will delete it later with guid_free (or whatever). Changing this at this point is probably out of the question.
Okay, I verified it. I created the following test: TEST (KvpValueTest, stupid_test) { KvpValueImpl v1 {1.1}; auto guid = guid_new(); std::cerr << "assigning " << guid << " to v1...\n"; v1 = KvpValueImpl {guid}; std::cerr << "finished with assignment\n"; } I also added the following to guid.cpp in guid_free: std::cerr << "Deleting " << guid << "\n" << std::flush; The output is assigning 0x1d60f10 to v1... Deleting 0x1d60f10 finished with assignment Deleting 0x1d60f10 And of course, a "double free or corruption".
(In reply to comment #56) > > That doesn't make any sense at all. First of all, KvpValueImpl doesn't have a > > default constructor so the first line won't even compile. > > It's pseudo-code :-). Change it to > > KvpValueImpl v1 {1.1}; > or > KvpValueImpl v1 {(int64_t)5}; > or whatever. That part doesn't matter. > > > If it did, it would > > be whatever was the default payload, not guid, that was deleted in the third > > line; the guid would be deleted when v1 goes out of scope. > > I agree that the first payload of v1 will be deleted, but the temporary created > by KvpValueImpl {guid} is what shouldn't be deleted. Since KvpValueImpl doesn't > know about move semantics, it isn't able to preserve the contents of the > temporary KvpValueImpl correctly, and it assigns a deleted guid pointer to v1. > > Now, as I'm writing this, I find it somewhat hard to believe, but that's the > meaning of the comment! I'll try to remember to take a look and verify thA > > > That's actually a > > feature, because otherwise guid would be leaked. It would be better if guid was > > on the stack, too, so the compiler could just move it around (I don't see > > anything in either class that would prevent the compiler from generating the > > implicit move constructors) or copy it. Oops, I'm wrong about that. The implicit move and copy constructors are suppressed; there's a KvpValueImpl(const KvpValueImpl&) and ~KvpValueImpl(). So one obvious solution is to implement KvpValueImpl(KvpValueImpl&&) and KvpValueImpl& operator=(KvpValueImpl&&) to do the right thing. But you should also implement KvpValueImpl& operator=(const KvpValueImpl&), which would let you assign correctly from an lvalue KvpValueImpl&. Obviously it should do the same thing as the copy operator. Either will avert the double-delete crash. Another approach, which I didn't try, was to somehow use shared_ptrs in the boost::variant. That might have other implications, particularly since for the near term we have to deal with passing raw ptrs back to C functions and might have issues with ref counts. > > A heap allocation is way more expensive > > than copying a guid, so there's no good reason to allocate it. > > A good deal of code depends on KvpValueImpl taking a pointer (to guid, or > glist, or Kvp_Frame, etc.) and (deeply) deleting the contents of that pointer > later. So the "good reason to allocate it" is because KvpValueImpl expects a > 'guid_new'ed guid, and it will delete it later with guid_free (or whatever). > Changing this at this point is probably out of the question. The point of the new implementation is to fix problems, and memory-management problems are high on the list of things to fix. That applies even if it affects much of the code in the rest of the program; it's just a bit more challenging because you have to design both a solution that works with what's there now and one that works the way you ultimately want it to so that you can transition the other code from the former to the latter. I've taken the liberty of adding the move constructor and operator= for both copy and move, and a test to show that creating a KvpValueImpl on the stack doesn't crash, and removed your comment since it's no longer necessary.
I think that's fine. Importantly, this scenario also works: KvpValueImpl getValue (GncGUID * guid) { return KvpValueImpl {guid}; } { auto guid1 = guid_new (); auto guid2 = guid_new (); KvpValueImpl v1 {guid1}; v1 = getValue (guid2); } guid1 is deleted when the temporary (returned from getValue) is destroyed, and g2 when v1 goes out of scope.
Created attachment 291132 [details] [review] kvpframeimpl test suite. Attached is a KvpFrame test suite. I didn't look at move and copy semantics for KvpFrameImpl. If you would like me to, feel free to say so. I'll be out of town for a week and an half, so I probably won't get time to look into it then, but I'll be glad to after I get back. I did realize that we don't need typedef struct KvpFrameImpl KvpFrame; or typedef struct KvpValueImpl KvpValue; Since there is only one struct that bears anything like either of these names, simply struct KvpFrame; in the (C) header file is sufficient, and rename KvpFrameImpl to KvpFrame. The attached patch doesn't do anything like this, but it struck me rather recently that there's no need to have two names for this struct!
Aaron, can I close this bug now, or do you have more KVP work in mind?
Closed. KVP rewrite completed in kvp-cleanup.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=736687. Please update any external references or bookmarks.