jlblancoc / nanoflann

nanoflann: a C++11 header-only library for Nearest Neighbor (NN) search with KD-trees
Other
2.26k stars 491 forks source link

Conflicts with multiple versions of nanoflann #212

Open teub opened 1 year ago

teub commented 1 year ago

I spent some time debugging an access violation, only to find out the executable wasn't instantiating the correct classes for nanoflann. Due to how c++ seems to work, the same template class could be defined at different places and there's no compiler error of any sort.

Anyway, long story short : could there be some define to specify a namespace and allow two versions of nanoflann to exist in the same program ? Of course I tried to do namespace something {

include "nanoflann.hpp"

} and it doesn't work for various reasons.

Also to preemptively answer why there are 2 different versions in my program : one of them comes from another 3rd party static library, which I'd rather not change.

Thoughts ?

jlblancoc commented 1 year ago

I think the main problem is having the two instances of the same library... :-) And more importantly, perhaps even using different build flags (gcc flags... see the compile full command line, e.g. VERBOSE=1 make). If it's not ensured that nanoflann is included with the same compiler flags (those regarding optimizations in particular), there is no way to make it safe to pass a reference or pointer of a kd-tree from one translation unit to another. If using different versions of nanoflann, things may be even worse if in between the number of fields in any of the used classes changed.

In short: I think using two namespaces would not be a solution, unless you can put together a toy example of what's your exact situation and why it's useful. The same header can be included in several files (translation units), and the instantiated objects passed from one to another, as long as the header is actually the same, and the compiler flags do not lead to change in the memory layout.

teub commented 1 year ago

In short: I think using two namespaces would not be a solution, unless you can put together a toy example of what's your exact situation and why it's useful.

Well I did : nanoflann is used by a 3rd party library I'm using(which I didn't know), and I'm also using it. I don't want to modify the 3rd party library's build system, but I can modify my own one : hence the need for changing the namespace.

I'm using the latest version of nanoflann but the 3rd party library is not.

I think it's a known pattern to allow changing the namespace of a library, see for example how they do it in MathGeoLib : https://github.com/juj/MathGeoLib/blob/master/src/MathBuildConfig.h (note : it could be done better, by allowing the define to be set by the build system : #ifndef MATH_NAMESPACE_NAME ...).

IMO the fact that a 3rd party lib is using nanoflann without me knowing is good, it means this repository is very popular :-) (and therefore the issue I raised is more likely to happen to others).

I have my own patch to handle the problem now, which I'll remove if you provide a configuration option for it. Thank you !

jlblancoc commented 1 year ago

Feel free of proposing a Pull Request with this feature and we'll review and iterate on top of it, please :-)