C++ and derived classes issue

[Geert Janssens]

On Friday 12 February 2016 17:15:41 John Ralls wrote:
> > On Feb 12, 2016, at 1:09 PM, Geert Janssens
> 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

Thanks for the detailed explanation John. It took me several readings to grok it.

I certainly like the idea of a set_trans_property to avoid different return types.

I had already attempted to use a template class instead, but ran into several issues (partly due 
my long hiatus in c++ coding).

I'm redoing it now based on your suggestions. Current code is here:
https://github.com/gjanssens/gnucash/blob/cpp/src/import-export/csv-imp/gnc-trans-props.cpp[1]
and
https://github.com/gjanssens/gnucash/blob/cpp/src/import-export/csv-imp/gnc-trans-props.hpp[2]


I'm still having a couple of issues:

1. The first is mainly lack of knowledge. I note you write a constructor like this:
GncTransProperty (T x, const char* p) : value{x}, property{p} {};

I'm not familiar with the ": value{x}, property{p}" notation. Is that a specific C++ notation to 
initialize private members ? Any caveats with it ? (I didn't come across an explanation in 
Meyers book so far, so I assume it's standard C++). Does it have a name I can google ?

2. I also note you are using "T x" in the constructor, suggesting the GncTransProperty is 
constructed from the properly typed value. However the importer so far only has a bunch of 
strings to process. The idea was exactly to let the GncTransproperty instances handle type 
conversion from the generic string to the proper internal type for later use. In this way that 
knowledge is encapsulated in the object that needs to know. If I forgo this, the calling code has 
to do the conversion, breaking the encapsulation and in fact removing much of the motivation 
to create the GncTransProperty class in the first place. So I have for now kept the more generic 
constructor
GncTransProperty (std::string val, GncTransPropType p)
This required specializations for each subtype of GncTransProperty we care about. 

3. Just so you can follow I'll also note that I can't do this:
template <typename T> class GncTransProperty
...
std::vector<GncTransProperty> vctp;
Based on the following StackOverflow topic,
http://stackoverflow.com/questions/16527673/c-one-stdvector-containing-template-class-of-multiple-types
I have hence chosen to write a pure virtual interface class GncTransProperty which is the base 
class of the GncTransPropImpl template class.

With this I can define
std::vector<std::unique_ptr<GncTransProperty>> vctp
which is the base class of GncTransPropertyList
and get on with my life.

4. Back to the real issues: if I would use g_object_set to set the date_posted for a transaction it 
will call different functionality inside the Transaction code than the importer is doing currently. 
g_object_set will eventually call xaccTransSetDatePostedTS where the importer has been calling 
xaccTransSetDatePostedSecsNormalized. The latter also sets a KVP (with the gdate only) where 
the former doesn't. I'm not sure if we really need that additional KVP, but don't like to pull too 
many strings at once either.

3. You propose to make all the possible import items properties of the Transaction GObject. I'm 
not really in favor of that as several of these import items are actually properties of splits to 
create. The name "Transaction" is used fairly liberally in the importer to mean a Transaction (in 
the engine sense of the word) together with its Splits. I don't think it's appropriate to add all of 
those into the Transaction engine object. On the other hand, having both a set_trans_property 
and a set_split_property again breaks the encapsulation. The calling object needs to know when 
to use which.