igraph-cpp

[szhorvat]

I've been planning to do something like this for a long time, to become an official part of igraph, but I just don't have the spare capacity. Would it make sense to work together? We'd have to find out if our goals align.

I used rudimentary RAII wrappers in the Mathematica interface of igraph, which are partly factored out as proof-of-concept here:

https://github.com/igraph/igraph-cpp/

[LTLA]

Oh wow. igraph-cpp seems to have exactly what I want. If I were to ignore your "experimental" disclaimer in the README, I think I could pretty much use your classes verbatim. In fact, I was even planning to write various STL-like methods today (begin(), data(), operator[], etc.), now I guess I'll just copy your code into my existing definitions.

Some more context: I have another library that uses raiigraph. Previous iterations of this library would only use these classes internally, so the RAII was just to make life more convenient for me. However, in the latest version of this library, I'm planning to expose these classes directly to my library's end users, so easier they are to manipulate, the better. Also, I'm happy to keep using the C library functions on the new classes via conversion operators, so the scope of my goals with respect to raiigraph (or igraph-cpp) is strictly limited to the convenience of working with the data structures.

TBH if you were to remove your disclaimer and add a version tag, it looks ready for downstream use to me. Not really sure what I could contribute at this point. Maybe clean up the CMakeLists.txt a bit so I can just FetchContent it easily, perhaps I could write some docs (though I'm not familiar with the igraph documentation system).

From reading your code, the only non-obvious thing that comes to mind is the macro-related fiddling in igraph_pmt.h and igraph_pmt_off.h. I barely use macros outside of header guards so this is quite neat to see, but it does make me wonder about what happens inside larger applications where other libraries might set macros of the same name. For example, I'm slightly surprised that VECTOR doesn't interfere with an existing definition in the R headers or Eigen. I suppose this concern is mostly theoretical given that I haven't actually run into any problems with the C libraries. ¯\_(ツ)_/¯

Anyway, if you have a plan to get igraph-cpp out of the experimental phase, I'll see what I can do to chip in.

[szhorvat]

it looks ready for downstream use to me

Unfortunately, it's not even close. I made many improvements today, but there are still many major issues. Feel free to copy any code though.

Some problems:

• igraph_vector_list_t: The function that deal with this return pointers to vectors that are still owned by the vector list, but are okay to modify. Can we wrap these? Additionally, there are functions like igraph_vector_list_push_back() that move ownership of the vector to the vector list. How can we handle these properly?
• igraph_t is different from vectors in that functions that create graphs must take an uninitialized igraph_t. If our RAII class doesn't support uninitialized graphs, then it must have a means to take over ownership. This is what I tried today, but I'm not entirely happy with it.

Overall, the proof of the pudding is actual usage. I would say: feel free to borrow code as you like and try it out. If you can give feedback or tips for improvement, that would be much appreciated. I don't expect this to be smooth sailing. I'm sure many usability issues will come to light.

With such feedback we can eventually make it final, but I expect that'll take quite a while.

[LTLA]

Hm... I see. Well, for what it's worth, I can say that what you've got is (close to) good enough for my use case.

(Also, the constructor to take ownership of an existing igraph_t looks fine to me; I do pretty much the same thing. You might provide even more safety by accepting igraph_t&& instead. In this case, the user must explicitly relinquish ownership of the igraph_t when constructing the RAII class, so there would be no surprises about what's going on.)

Would you accept some PRs? Some thoughts for improvement include:

• Get the CMakeLists.txt ready for downstream consumption via FetchContent and/or find_package().
• Add separate class for views (analogous to std::span in C++20).
• Use nicer names:
  • More descriptive namespace name, e.g., ig to igraph to avoid potential future headaches re. conflicts.
  • No need for ig prefix in the classes, as that's redundant with the namespace.
  • Vec to Vector and Mat to Matrix, to be more consistent with the C library's naming.
• Probably add some GitHub Actions to test all the above changes.

No pressure, though. I'm happy to keep on using raiigraph for the time being.

[szhorvat]

Would you accept some PRs?

Yes, absolutely! Most suggestions sound good to me.

• More descriptive namespace name, e.g., ig to igraph

Let's call the namespace ig for the moment to avoid conflict with https://github.com/ntamas/igraphpp/

---

Since you are actually using it in a project (no one else is at the moment), you are in a good position to judge what changes are needed.

All I ask is that we should keep the door open to significant modifications (at least until the vector_list thing is sorted out). So I don't want to make a versioned release for the moment. Instead, I'd suggest vendoring the code in your project, e.g. through a Git submodule.

PRs are very welcome! Since this is a highly experimental project that's not currently used by others, and since development should be usage-driven, feel free to propose major changes.

[szhorvat]

I made some changes to try to enable igraph_vector_list_t support. The way this works is that the wrapper object can act either as the owner of the wrapped data, or just an alias/reference to it. I find this a little ugly and I'm not entirely happy, but I don't have a better idea. igCapture() and igAlias() control whether the wrapper takes ownership of or just references/aliases a raw igraph data structure. Suggestions are welcome.

igVecList still needs its own iterator type, and the appropriate push_back() method that transfer ownership appropriately.

I agree with changing the ig naming and just using namespaces, but that can come later.

[szhorvat]

Right now this is set up so that igRealVec can be automatically converted to igraph_vector_t *. I'm a little uneasy as I'm not sure where this may go wrong. What do you think? How about overloading the & operator, so that if we have igRealVec v, we have to explicitly write &v to get the igraph_vector_t * pointer? This is still concise and easy to write.

[LTLA]

Sorry for the late reply, was trying to finish up some things before I get onto this.

Right now this is set up so that igRealVec can be automatically converted to igraph_vector_t *.

I was planning to do the exact same thing, even before I looked at your code. It is pretty convenient to be able to pass the RAII classes directly to the C functions without an extra coercion step.

The only risk that comes to mind is when the C functions take a pointer to an uninitialized igraph_..._t object, but the RAII classes have already initialized their owned object, causing a memory leak. In these scenarios, though, I don't think there's much syntactically that can be done to make the RAII classes safer. An uninformed user could almost as easily create the same memory leak with the C API if they didn't read the documentation for those functions.

In any case, if it still bothers you, you might consider following std::shared_ptr's lead and implementing a get() function to obtain the raw pointer. Overloading & would be very surprising behavior to me, especially in heavily templated code where I would be expecting base operators like & to just do their usual thing regardless of type.

I made some changes to try to enable igraph_vector_list_t support.

Having never used this at all, I can't really give much of an informed opinion, I'm afraid. Here's some general thoughts:

• In other contexts (Python and Javascript), I've had real headaches with complex objects silently sharing ownership of their components. Changes to components in one part of the app/workflow would cause problems somewhere else because of modify-by-reference. So I would lean towards avoiding references/aliases that aren't explicit.
• A very major advantage of the RAII classes - at least how I've implemented them in raiigraph, and it seems the same in igraph-cpp - is that each instance has full ownership of an initialized igraph object. There's no ambiguity for users, which makes life a lot easier, e.g., no need to check for NULL, no need to worry about side effects. I think we should aim to stick to this philosophy, even if it comes at the cost of some copying/inefficiency.
• And if a user really needs performance, I think it's fine to just tell them, "sorry, but you'll have to fall back to the C API to do that". FWIW I regularly use raw pointers in my code when I want the STL to get out of the way.