(I don't know why but my mail got mangled while going through the intertubes.
I'll try to resend. Hopefully this time it comes through ok.)
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.
_______________________________________________
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
