google / s2geometry

Computational geometry and spatial indexing on the sphere
http://s2geometry.io/
Apache License 2.0
2.32k stars 306 forks source link

Types and functions should be in a namespace or consistently prefixed to avoid ODR violations #386

Open joka921 opened 3 weeks ago

joka921 commented 3 weeks ago

Currently, the S2 library is not wrapped in a namespace. Additionally, most of the class names contain "S2" somewhere in their name, but not all of them. In particular, there are types called Decoder and Encoder in the public namespace defined in util/coding/coder.h. This leads to one-definition-rule violations when S2 is linked together with other code that also uses classes of the same name (in fact I ran into such problems).

In my opinion, there are two possibilities to mitigate this problem:

  1. Consistently use a namespace for everything inside S2.
  2. Consistently use "S2" as a prefix, suffix, or infix of all names of type, i.e renaming the Encoder and Decoder classes to S2Encoder and S2Decoder or using a similar naming scheme.

Please let me know, which variant you would prefer, and whether I should prepare a PR or whether you want to fix this yourselves, as it probably would break existing code and would require adaptations in documentations etc.

smcallis commented 2 weeks ago

Definitely on my radar but it's a larger refactoring than just the open source because we have many internal clients that would have to be ported, and it's never been a high enough priority to do it.

jmr commented 2 days ago

We'll fix this ourselves.