Created attachment 302609 [details] [review]
Refactor qof session

Attached is the patch. Logic is moved into C++ functions and wrapped in C functions.

Backend should be easy to refactor at this point, and after that's done (as it's done?) qof session can be rewritten to use the C++ interface of backend. (That hasn't happened much at this point of the refactor, and to me it's exciting.)

I eagerly await criticism and suggestions!

Comment on attachment 302609 [details] [review]
Refactor qof session

RAII. The constructor should should take care of doing all of the setup (with other static functions as appropriate of course) and the destructor doing all of the cleanup.

A style comment: You don't need to use this-> to refer to object members. Just name them, so e.g. just backend rather than this->backend.

Except that we want member variables to be named with a leading m_, so it should be m_backend.

Provider list should be a std::vector<QofBackendProvider>, not a GSList. It should be a class (i.e. static) member of QofSession, not a stack variable in LoadBackend.

Prefer auto when possible. E.g., auto prov = static_cast<QofBackendProvider*>(p->data) -- though that particular example won't apply when the provider list is a std::vector, because the std::vector will preserve the type.

access_method should become an enum. Doing an (fancy) strcmp for something like that is dumb.
Rather than swapping books manually, use std::swap. There are probably other opportunities to use c++stdlib functions. Do so whenever possible.

Regardless of whether QofSessionImpl is a class or a struct, member variables should be private. 

Consider as well separating the C and C++ sections of the file, or even putting the C wrappers in a separate file. Yes, it will make the patch set a bit uglier, but it's much easier in the future to see what's going on in the implementation if all of the C++ class functions are together, ideally in the same order as in the declaration, and the C functions are similarly grouped.

There's undoubtedly more, but I think you get the idea.

I read you loud and clear.

There is one thing that isn't clear to me. "The constructor should take care of doing all ... and the destructor ..." Are you looking specifically at member function calls here (in the constructor, clear_error, and in the destructor, destroy_backend)?

Comment on attachment 302609 [details] [review]
Refactor qof session

I think I meant the QofBackend* member backend (would be better renamed as m_backend). You're creating it in QofSessionImpl::load_backend() and deleting it with QofSessionImpl::destroy_backend(), both of which are public functions. Don't do that. Acquire ownership in the constructor and release in the destructor. If that rule isn't familiar to you, read up on "Resource Acquitisition Is Initialization (RAII)".

Created attachment 327536 [details] [review]
Convert qofsession to c++

The newly attached patch accomplishes much of what was requested. There are strong assumptions (mostly in src/engine) about how qofsession works. I spent a good deal of effort using RAII as suggested, but the ensuing rewrite of the engine/session interaction was waxing great. Let me know if you would like me to move in that direction with more force, or if it would be better to wait until src/engine is targeted more directly.

Access_method was left as GList rather than std::vector<access_method> for a similar reason: it's used in many C places that won't be able to understand the std::vector, nor the desired scoped enumeration (http://en.cppreference.com/w/cpp/language/enum).

On the one hand, I would like to make smaller changes in each commit, confining my changes as much as possible; on the other hand, we're engaged in a near-total rewrite of the code. Let me know if I'm being to restraintive. If we would be happy to have me branching out more and making farther-reaching changes, it would probably better if we switch to an earlier recommendation for taking code changes: github. That way, the commits can be smaller and more numerous.

A conversation on IRC concluded that this work will continue through github pull requests.

GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=748669. Please update any external references or bookmarks.