lemire / FastPFor

The FastPFOR C++ library: Fast integer compression
Apache License 2.0
883 stars 124 forks source link

Avoid multiple redefinition when including codecfactory.h from different .cpp files #91

Closed pps83 closed 1 year ago

pps83 commented 1 year ago

fixes #83

pps83 commented 1 year ago

@lemire I have a question. Your github seem to have multiple int compression libs: https://github.com/lemire/SIMDCompressionAndIntersection https://github.com/lemire/streamvbyte https://github.com/lemire/simdcomp

What's the relationship? It seems that fastpfor includes streamvbyte (is it older version, does it need to be updated with upstream?). What about simdcomp vs fastpror?

lemire commented 1 year ago

This PR would effectively entice users to use the same codec instances throughout their application, and in particular across different threads. This goes contrary to our documentation which states that users should have different codecs per threads to avoid data races.

lemire commented 1 year ago

Your github seem to have multiple int compression libs:

Indeed. If you find that the documentation of one of them, or of many of them, is inadequate, please open an issue specific to that library explaining why the documentation is inadequate, and ideally, making proposal regarding how the documentation of that library be improved.

lemire commented 1 year ago

If you have multiple definitions, then your code is almost surely unsafe.

lemire commented 1 year ago

We are going to go with a simpler approach that should be safer: https://github.com/lemire/FastPFor/pull/92

pps83 commented 1 year ago

We are going to go with a simpler approach that should be safer: #92

In my own project I have wrappers that use these IntegerCODECs as temporary vars:

void compressStreamvbyte(const uint32_t* in, const size_t length, uint32_t* out, size_t& nvalue)
{
    StreamVByte().encodeArray(in, length, out, nvalue);
}

Most coders don't have any state, so, this should be ok.

lemire commented 1 year ago

Most coders don't have any state, so, this should be ok

I agree.