Consider removing ckdb namespace

[kodebach]

Currently, we use the namespace ckdb for some, but not all of our C libraries. In general it is very unclear, when the namespace should be used an when it shouldn't be.

Personally, I think we should just remove it and not use a namespace in our C functions. If someone using C++ really needs to put e.g. the contents of kdb.h into a separate namespace to avoid conflicts, they could still use:

namespace ckdb {
#include "kdb.h"
}

The problem with using the ckdb namespace are quite obvious from all the chaos it caused in #2976.

[dominicjaeger]

When writing my only C++ file in this project src/tools/kdb/cmerge.cpp I was confused by the difference between ckdb and kdb. As a consequence I guessed and checked if the compiler threw an error sometimes.

Sure, I had nearly 0 experience with C++, but future contributors might be in the same boat. So if this might make things easier from your experience, I support it.

[markus2330]

The problem with using the ckdb namespace are quite obvious from all the chaos it caused in #2976.

I think this was mostly misunderstanding of C++ namespaces. But yes, as many of us are beginners in C++ it is good to keep it simple.

namespace ckdb {
#include "kdb.h"
}

Are you sure this works? IIRC there were problems with enums.

The problem with using the ckdb namespace are quite obvious from all the chaos it caused in #2976.

I think this was mostly misunderstanding of C++. But yes, as many of us are beginners in C++ it is good to keep it as simple as possible.

If @Piankero manages to remove all ckdb namespaces (maybe in a separate branch, so that we still have #2976 just in case) we can do it. Otherwise I would not waste too much time on this.

In general it is very unclear, when the namespace should be used an when it shouldn't be.

Alternatively, we simply clarify, that every public header file declares all functions in:

#ifdef __cplusplus
namespace ckdb {
extern "C" {
#endif

[kodebach]

Let me rephrase my point: What was the intention behind the ckdb namespace? What is the benefit of using it?

IMO there is no benefit. All symbol names have to be globally (including outside of Elektra) unique anyway, otherwise we run into problems in C.

[kodebach]

Are you sure this works? IIRC there were problems with enums.

As far as I understand it, #include basically copies the referenced file into the current one. Whether the namespace declaration is in one file or the other shouldn't matter, as the preprocessor has no concept of namespaces.

[markus2330]

Let me rephrase my point: What was the intention behind the ckdb namespace? What is the benefit of using it?

That the global namespace is not cluttered. It is not an ideal situation anyway, as the symbol names are not mangled anyway. But at least there are no problems when e.g. ksNew implemented in C++ exist.

IMO there is no benefit. All symbol names have to be globally (including outside of Elektra) unique anyway, otherwise we run into problems in C.

Yes, for C there is obviously absolutely no benefit.

As far as I understand it, #include basically copies the referenced file into the current one. Whether the namespace declaration is in one file or the other shouldn't matter, as the preprocessor has no concept of namespaces.

Of course, but then the whole content gets into the namespace. Currently only the functions (not the enums) are in ckdb.

[kodebach]

But at least there are no problems when e.g. ksNew implemented in C++ exist.

kdb.hpp should just include kdb.h inside a namespace ckdb { } like shown above.

Currently only the functions (not the enums) are in ckdb.

That makes the whole thing even more confusing....

I get that it is so you can use the enum values in C++, but that also pollutes the global namespace.
It would be best to use enum classes for C++. It's of course more complicated, since Elektra's "enum types" are in fact all typedef int, because otherwise ++ doesn't work in C++.

We could introduce a macro ELEKTRA_ENUM similar to this:

#ifdef __cplusplus
#define ELEKTRA_ENUM(name, ...)                                                                                                            \
	enum class name                                                                                                                    \
	{                                                                                                                                  \
		__VA_ARGS__                                                                                                                \
	};                                                                                                                                 \
                                                                                                                                           \
	name & operator++ (name & val)                                                                                                     \
	{                                                                                                                                  \
		val = static_cast<name> (static_cast<int> (val) + 1);                                                                      \
		return val;                                                                                                                \
	}                                                                                                                                  \
                                                                                                                                           \
	name & operator-- (name & val)                                                                                                     \
	{                                                                                                                                  \
		val = static_cast<name> (static_cast<int> (val) - 1);                                                                      \
		return val;                                                                                                                \
	}                                                                                                                                  \
	/* define other required operators */

#else
#define ELEKTRA_ENUM(name, ...)                                                                                                            \
	typedef enum                                                                                                                       \
	{                                                                                                                                  \
		__VA_ARGS__                                                                                                                \
	} name;
#endif

ELEKTRA_ENUM (elektraNamespace, KEY_NS_NONE, KEY_NS_CASCADING, KEY_NS_SPEC);

We could also just put the enums into a separate kdbenums.h and include that before kdb.h and outside of any namespace in kdb.hpp. The include guard would than mean that the enums live in the global namespace.

---

The C API shouldn't really care about any C++ concepts beyond extern "C" (and even that could be put into the C++ binding).

[markus2330]

That makes the whole thing even more confusing....

The logic behind that is that the enum values already have a prefix and do not need to be in a namespace (as their name should not clash already).

It would be best to use enum classes for C++.

Yes but then you do not need the KDB_ or ELEKTRA_ prefix. So it might be better to reimplement the enums as enum classes in the C++ binding. (But this would be a new feature not needed for 1.0.)

The C API shouldn't really care about any C++ concepts beyond extern "C" (and even that could be put into the C++ binding).

Yes, this is a completely valid viewpoint, the question is how time consuming it is to get rid of ckdb. I would like to avoid that someone spends much time on such little annoyances: they are more or less matter of style unless you find something in the standard which prohibits to have namespaces for something that is in extern "C" (iirc I could not find anything about that. And all compilers do what I would expect of this case.).

[kodebach]

The logic behind that is that the enum values already have a prefix and do not need to be in a namespace (as their name should not clash already).

All C symbols use prefixes to be unique. If someone wanted to use the name kdb::KEY_NS_SPEC for example we would have the exact same problem as with functions.

But this would be a new feature not needed for 1.0.

Well, it is a breaking change, so 1.0 would be good opportunity. (If it can be done quickly of course.)

which prohibits to have namespaces for something that is in extern "C"

AFAIK extern "C" mostly affects function linkage, and doesn't restrict the language in any way. In any case, the namespace could also wrap around extern "C" instead of the other way.

[markus2330]

All C symbols use prefixes to be unique.

Unfortunately not. kdb is a not so unique (it might conflict with Kerberos). But of course the same can be said for KEY_ or KDB_.

Well, it is a breaking change, so 1.0 would be good opportunity. (If it can be done quickly of course.)

I agree.

AFAIK extern "C" mostly affects function linkage, and doesn't restrict the language in any way.

Function linkage limits the language, e.g. overloading. So there might be also restrictions in namespaces (beside the obvious restrictions).

[markus2330]

Ok, lets remove ckdb as low-priority task (if @Piankero can do it within reasonable time frame).

[ghost]

I do not think that I am the appropriate person to do this task as I am not used to these namespace concepts along with their problems. I am already heavily struggling with #2976 and cannot get rid of the namespace errors despite suggestion of @kodebach . The problem is that locally everything works and compiles but not on the build server.

[markus2330]

By removing all these ckdb namespaces you obviously could not get these errors anymore. So it might even help you if you remove them.

[ghost]

This issue is unrelated to visible error messages and low priority because of high complexity.
As discussed in the Elektra meeting I may unassign myself.

[markus2330]

@Piankero I agree. And I think we will leave the ckdb namespace as it is.