Libelektra: keyMeta in C++

[kodebach]

Since #2983 is now implemented and we have KeySet * keyMeta (Key * key); it would be nice to have a similar function in C++. I propose an overload for Key::getMeta:

inline kdb::KeySet kdb::Key::getMeta () const
{
    return kdb::KeySet (ckdb::keyMeta (this->key));
}

However, I don't know where to put this function the whole binding uses inline functions (which is good) and therefore is header-only. But I can't put the function into key.hpp because that file doesn't know about kdb::KeySet. If I put the function somewhere else, just using #include <key.hpp> won't work anymore. Anybody with more C++ knowledge got any ideas?

[markus2330]

Yes, if you directly wrap it you get ugly circular references. It is possible to write inline functions also outside the class but then we would still have the problem that the C++ KeySet takes ownership.

Maybe it is better/enough if we provide a Meta Iterator (together with the get/set meta functions we already have). KeySetIterator could be included key.hpp as it only needs a forward declaration to the KeySet (it only holds a reference). But for creating this iterator, a similar problem arises which could be fixed by making KeySetIterator work with ckdb::KeySet instead (no functionality of kdb::KeySet is used there anyway).

Btw. I think we can remove the macro ELEKTRA_WITHOUT_ITERATOR which would complicate this endeavor even more.

@manuelm would such an iterator then directly work for the SWIG bindings?

Another problem arises with keyMeta: When we remove the key*Meta functions, the C++ binding either depends on libelektra-meta (@manuelm is this a problem for the SWIG bindings?) or we need to reimplement the functions within the C++ binding (which should actually not be a big deal as it is usually only a one-liner).

[manuelm]

SWIG can only wrap methods inside the class/struct block.

The iterator should work, yes.

No. Just link them to ...-meta too.

[kodebach]

which should actually not be a big deal as it is usually only a one-liner

If they are a one-liner in C as well, we should write them as static inline in kdbmeta.h (or another header) then nobody needs to link against libmeta for these functions.

the problem that the C++ KeySet takes ownership

I assume you mean that the destructor calls ksDel on the meta KeySet? Wouldn't that also be a problem if someone decides to do that in C? If I do ksDel (keyMeta (key)) the key always has invalid metadata no matter the language used.

Maybe we put a flag on metadata KeySets that disables ksDel. keyDel would then remove the flag before calling ksDel.

[kodebach]

Maybe it is better/enough if we provide a Meta Iterator

At least for my use case that wouldn't work. I wanted to use ksCut on the metadata. I ended up using

KeySet meta (ckdb::keyMeta (key->getKey ());

Come to think of it a subclass MetaKeySet with a constructor that takes a single kdb::Key should work, shouldn't it? It wouldn't be very idiomatic, but I don't see why it wouldn't work (except for the ownership stuff).

[markus2330]

KeySet meta (ckdb::keyMeta (key->getKey ());

Maybe a key->getMeta() returning a ckdb::KeySet would be enough? Then one could use:

KeySet meta (key->getMeta ());

Come to think of it a subclass MetaKeySet with a constructor that takes a single kdb::Key should work, shouldn't it?

The Key(Set) is not designed for subtyping (no virtual constructor). @manuelm tried to build something similar with a subclass of Key (FrozenKey). We dropped the idea later as there was an easier way with locking.

It wouldn't be very idiomatic, but I don't see why it wouldn't work (except for the ownership stuff).

The ownership stuff could be fixed this way. The MetaKeySet constructor could increment the references and not steal the ownership. But the cyclic reference would not go away, as MetaKeySet inherits from KeySet you still need the order Key -> KeySet -> MetaKeySet and Key would not be able to return a (Meta)KeySet.

[kodebach]

Then one could use: [...]

Thats not much shorter than calling ckdb::keyMeta and if we still get a ckdb::KeySet, I don't see how this would help much. In other languages this might make sense, but in C++ its trivial to call a C function, so if the result value is the same and calling the wrapper is not much shorter, there is not much point in having a wrapper.

as MetaKeySet inherits from KeySet you still need the order Key -> KeySet -> MetaKeySet and Key would not be able to return a (Meta)KeySet.

The idea was to not have a MetaKeySet Key::getMeta(), but instead require to the user to call MetaKeySet (key). Then there would be no need to include keyset.hpp from key.hpp.

---

Another solution for the circular references would be this (haven't tested it but I think it should work):

namespace kdb
{

class KeySet;

class Key
{
     // ...

    inline KeySet getMeta();
};

// ...

}

#include "keyset.hpp"

namespace kdb
{

inline KeySet Key::getMeta()
{
    return KeySet (ckdb::keyMeta (key));
}

}

Since both headers have include guards, there won't be a true circular reference. Since we pre-declare class KeySet; we can use it in the prototype and because we #include "keyset.hpp" before actually defining Key::getMeta, we can safely use the constructor inside Key::getMeta.

This of course means that #include <key.hpp> and #include <keyset.hpp> become equivalent, but I don't think that matters.

[markus2330]

calling the wrapper is not much shorter

KeySet meta (ckdb::keyMeta (key->getKey ());
KeySet meta (key->getMeta ());
MetaKeySet meta (key);

Since both headers have include guards, there won't be a true circular reference.

Yes, something like this works in C++ (e.g. when using references or pointers in the signature) but @manuelm said that SWIG might have problems with this approach.

[manuelm]

SWIG has problems with methods defined outside of the class. https://github.com/ElektraInitiative/libelektra/pull/3133/commits/22dad82fd9cd78d9b6fe4b65c61145640776beaf#diff-52df99526c9295a142ee9028a0d15759

Don't see any immediate problems with @kodebach code example above

[markus2330]

SWIG has problems with methods defined outside of the class.
Don't see any immediate problems with @kodebach code example above

Ahh, ok. You mean a method/function cannot be declared outside the class. Defined outside is also the method getMeta in @kodebach example.

https://github.com/ElektraInitiative/libelektra/pull/3133/commits/22dad82fd9cd78d9b6fe4b65c61145640776beaf#diff-52df99526c9295a142ee9028a0d15759

Your operator== is not completely synonym to the previous (outside) operator ==. It breaks sthg_convertible_to_KeySet == KeySet, as now the left-side needs to be a KeySet.

But having a working == operator in Python is more important then this special case in C++.

[manuelm]

err yeah: s/defined/declared/g.

sthg_convertible_to_KeySet

e.g. the low-level ckdb::KeySet? Yeah you can always use KeySet(ckey) == ....

[kodebach]

Your operator== is not completely synonym to the previous

I'd even argue the new version is better, it might be very unexpected, if sthg_convertible_to_KeySet is silently converted into a KeySet for the ==.

[stale[bot]]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions :sparkling_heart:

[markus2330]

A nice 2.0 task, no hurry to do this.