-
Notifications
You must be signed in to change notification settings - Fork 807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Business objects cpp #1985
base: stable
Are you sure you want to change the base?
Business objects cpp #1985
Conversation
ecdcba5
to
b3edbd7
Compare
b3edbd7
to
3bb39bf
Compare
some .c were using 0 which didn't map to a valid enum. I've modified behaviour slightly to avoid them. EDIT: they're causing PERR messages in a regular datafile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing technically wrong here, but it has a lipstick-on-a-pig feel to it. The only thing you're changing to C++ is using std::optional to replace a boolean value pair. What's your ultimate goal?
libgnucash/engine/gncEntry.h
Outdated
GNC_PAYMENT_NONE = 0, | ||
GNC_PAYMENT_CASH = 1, | ||
GNC_PAYMENT_CARD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you increment by 1 you need only assign the first one, and 0 is the default so you can just add the 0 enum and skip the initializer, e.g.
typedef enum
{
GNC_PAYMENT_NONE,
GNC_PAYMENT_CASH,
GNC_PAYMENT_CARD
} GncEntryPaymentType;
libgnucash/engine/gncTaxTable.h
Outdated
{ | ||
GNC_AMT_TYPE_DISABLED = 0, /** no tax */ | ||
GNC_AMT_TYPE_VALUE = 1, /**< tax is a number */ | ||
GNC_AMT_TYPE_PERCENT /**< tax is a percentage */ | ||
} GncAmountType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are remarkably useless comments.
Main goal is to benefit from C++ stricter type checking etc. |
I don't think you can do that incrementally: Polymorphism in both GObject and QOFObject depends on passing and casting |
I think there's some benefit to incrementally converting more files to c++ -- it means the next milestone can be "all of libgnucash is now c++". It means more the headers can be .hpp and have less |
No, there's a huge yawning chasm between "compiles with the C++ compiler" and "is C++". The goal is emphatically not to get libgnucash to compile with the C++ compiler. There's no real benefit to that. There's some benefit to rewriting functions to use C++ std algorithms and using STL containers instead of GLib ones, but even that doesn't really help solve our main problems of memory management errors and spaghetti code. Plus those algorithms and containers work much better when they're used with real C++ objects.
That requires the consumer code also to compile with the C++ compiler: Swig files, Gui files, import-export files. That's a lot of extra work that can be made less by reducing the API and pushing the specialization out to the user that needs it. |
No description provided.