C++ and derived classes issue

[John Ralls]

> On Feb 12, 2016, at 1:09 PM, Geert Janssens <geert.gnucash at kobaltwit.be> wrote:
> 
> Hi,
> 
> While working to convert the csv importer to c++ I've come across this issue:
> 
> At some point the importer code wants to keep track of a set of values that can be used to 
> create a transaction from. The transactions can't be created immediately in that part of the 
> code because user manipulations in the GUI can change the interpretation of these values.
> 
> The c code used a glist to keep a list of these values (or "properties), each value being 
> encapsulated in a struct that also keeps some meta information.
> 
> For reference the old code starts here:
> https://github.com/gjanssens/gnucash/blob/cpp/src/import-export/csv-imp/gnc-csv-model.c#L623
> 
> To avoid code duplication, I chose to replace the struct with a small hierarchy of classes, the 
> base class being GncTransProperty, which handles the common parts and derived classes for 
> each type of value that can exist:
> * one to keep track of an account (Account *),
> * one to keep track of strings (std::string),
> * one to store a time value (time64)
> * and lastly one to keep an amount (gnc_numeric).
> 
> I also chose a generic base class to allow the instances to be stored in a std::vector. Which 
> properties get stored will depend on the user choices in the GUI. (I say "will" as that part is not 
> set up just yet).
> 
> Current attempt of the code is here
> https://github.com/gjanssens/gnucash/blob/cpp/src/import-export/csv-imp/gnc-trans-props.cpp
> and
> https://github.com/gjanssens/gnucash/blob/cpp/src/import-export/csv-imp/gnc-trans-props.hpp
> 
> Unfortunately this code fails to compile. The error I don't know how to handle is this one:
> src/import-export/csv-imp/gnc-trans-props.cpp:232:10: error: 
> 
> And similar errors for each specialized class I defined.
> 
> I do understand the reason for this error: return type Account * is not the same type as void * 
> as in the virtual function in the base class and I explicitly stated I want to override a virtual 
> function (as per Meyers Effective Modern C++ recommendation).
> 
> What I'm unable to figure out is how to do this properly. The code that will eventually call this 
> function will need to get the Account to create a split. It will also need to call a get_value on 
> another property to get, say the amount (a gnc_numeric). So depending on the property being 
> stored, I need another return type. Or don't I ?
> 
> What's a good idiom to encapsulate this information until the point where the differentiation is 
> relevant (only when storing a value or retrieving it) ?

First the compiler error:
Line 232 is 
Account *GncTransProperty::get_value () const
but the declaration of GncTransProperty::get_value() is void*, so the compiler errors. You have 
Account *GncTransPropertyAccount::get_value () const at line 244, which is correct. The local solution would be to either make the definition match the declaration or (better) make GncTransProperty a pure virtual class with no definitions at all, not even a constructor.

What you've forgotten is that foo->get_value() calls the get_value() implementation for the class that foo was constructed as, even if it was passed in as a parent class ptr. More concretely,

void foo (GncTransProperty* bar)
{
    Account *baz = static_cast<Account*>(bar->get_value());
    ...
}

auto waldo = new GncTransPropertyAccount();
auto pepper = new GncTransPropertyString();
foo(waldo);

Will work, but 
foo(pepper);
will compile but will behave unexpectedly because it will try to treat a std::string* as an Account*.

I think that what you really want to do is

template <typename T> class GncTransProperty {
public:
  GncTransProperty (T x, GncTransPropType p) : value{x}, property{p} {};
  T get_value() const { return value; }
  GncTransPropType get_property() const { return property; }

private:
  T value;
  GncTransPropType property;
}

That does everything you want in 10 lines and avoids the evil void*.
Even better (at least until Transaction is converted to C++) would be to expand its property list to include all of the things that the importer will set. Then you can do even more, and get rid of the second switch statement:

template <typename T> class GncTransProperty {
public:
  GncTransProperty (T x, const char* p) : value{x}, property{p} {};
  void set_trans_property (Transaction *txn) const {
	GValue val = G_VALUE_INIT;
	g_value_init_from_instance (&val, G_POINTER(value));
	g_object_set_property (G_OBJECT (txn), property, &val);
 }

private:
  T value;
  char *property;
}

You'll have to specialize the set_txn_property implementation for string, time64, and probably gnc_numeric since they're not GObjects. The specialization code looks like:

template <> class GncTransProperty <std::string>{
public:
  GncTransProperty (std::string& x, const char* p) : value{x}, property{p} {};
  void set_trans_property (Transaction *txn) const {
	GValue val = G_VALUE_INIT;
	g_value_set_string (&val, value.c_str());
	g_object_set_property (G_OBJECT (txn), property, &val);
 }

private:
  std::string value;
  char* property;
}

With that and the changes in Transaction.c, the loop in GncTransPropertyList::to_trans() can be reduced to 
std::for_each(this->begin(), this->end(), [](auto elem){elem.set_trans_property();});

BTW, except for the lambda and the curly-bracket initializers that's all C++98.

Note that replacing get_type and get_value with set_trans_property would work with a class hierarchy, too, because it gets rid of the return-type problem with get_value. The template version runs slightly faster because the functions aren't called by vtable. The template version is also more flexible: The base template can handle any GObject type without specialization. It should also be possible once Transaction is changed to C++ to create a more type safe (never mind faster) way to handle properties to get rid of the specializations too because we should be able to say txn->set(property, value).

Regards,
John Ralls