C++ namespace

[iboB]

Allow the definition of a custom C++ namespace. Closes #705

[iboB]

I just converted it to a draft.

It just occurred to me that there's another issue to resolve with this.

The PR as it is keeps extern "C" as is. This means that the symbols will get C mangling (_XXH64_update) regardless of the namespace. This means that they can still be defined in a C compilation unit, which is perhaps a good thing, but also that they will clash with existing symbols of the same name if linked in the same module, which can also be a good thing if the user means to mix C and C++ and only have one copy.

However a C++ programmer might want to mix a version of xxhash in the namespace with their C++ code with another which is in the C code of the same module (and not in the namespace) or with another which is in another namespace and be surprised when they get linker errors, or worse: hidden ODR violations.

I personally think that the current state is OK as long as it's documented. C and C++ can use the same symbols with or without a namespace.

@Cyan4973 what do you think? Should I keep it as is?

[Cyan4973]

I'm a bit confused. I initially thought you were looking for a way to avoid xxHash's public interface to "leak" into the global namespace. But this PR keeps C symbols names, and nonetheless concludes that it's "probably okay as long as it's documented".

So I'm trying for figure out what's the main goal.

[iboB]

The PR as it is is perhaps too influenced by my particular use case.

I am creating a shared library and I am only exporting C++ classes from it (the rest of the symbols have a hidden visibility). I don't want to allocate memory for the xxhash state, so the C++ classes have state members and for this they include xxhash.h with XXHASH_STATIC_LINKING_ONLY. There is no danger in keeping extern "C" as there is no danger of clash. If one uses my shared library with another version of xxhash there will be no definition clash even if they are included in the same compilation unit as the xxhash state structs are in a namespace.

Also in this scenario code autocomplete won't suggest xxhash symbols in the global namespace which, at least for me, is a significant benefit.

It is true however that this is not the only use case ever and other uses may suffer from extern "C" being kept as mentioned above.

To prevent this one would need to make extern "C" and the namespace either-or:

#if defined(__cplusplus)
#  if defined(XXHASH_CPP_NAMESPACE)
// open c++ namespace
#  else
extern "C" {
#endif

... but this would require XXH_IMPLEMENTATION to be defined in a C++ compilation unit. This is incompatible with the existing xxhash.c and will lead to undefined symbol linker errors if a C compilation unit is used. This is perhaps better, as even though errors will exist, they will not be hidden from the users. I can do that.

The behavior for my use case will still be achievable if one wraps all inclusions of xxhash.h in extern "C" { }. This at least is possible unlike wrapping them in namespace X { }

Ideally a configuration would allow all of these, but then the documentation would be overwhelmed by this relatively obscure feature and its invariants.

So, I can change the namespace wrap as in the example above and add documentation about XXHASH_CPP_NAMESPACE emphasizing that if it is used xxhash needs to be compiled as C++ and that xxhash.c is unusable in such a scenario.

However I acknowledge that this is a relatively obscure configuration and it will be quite understandable if you don't want it the codebase. In which case I'll just close the PR and that will be that

[iboB]

I added the code part of my suggestion sans the documentation. I'm going to use if for my purposes anyway.

[GitMensch]

With the current "either namespace or C extern" solution - is there a reason to keep that as work-in-progress?

[iboB]

@GitMensch

My intention was to document this in the readme if the suggestion is approved, but I now realize that I didn't express it clearly enough.

I'm marking it as ready for review. Docs can come even after this is merged (if it does)

[Cyan4973]

I'm mildly annoyed by the nb of places where this PR needs to CLOSE and OPEN the CPP_NAMESPACE. It happens essentially everywhere there is a #include statement. This subtly increases maintenance burden: in the future, any #include modification will have to also pay attention to the CPP_NAMESPACE topic.

Ideally, I would prefer not to, but that being said, I don't have any better option to propose...

[Cyan4973]

Closing, I'm concerned by the increased maintenance load that this PR would introduce notably around the many #include statements and other hard-to-follow potential impact on closing braces,

while the benefits are unfortunately unclear to me. This makes the cost / benefit evaluation unfavorable.