paleolimbot / s2geography

Simple features (ish) for s2geometry
Apache License 2.0
33 stars 4 forks source link

Remove absl dependency? #5

Open benbovy opened 1 year ago

benbovy commented 1 year ago

While inspecting the code, it looks like the only absl feature currently used in s2geography is absl::make_unique.

@paleolimbot, do you think that we can remove absl as a direct dependency of s2geography and use std::make_unique instead?

I know it is not a big deal since absl is already a dependency of s2geometry, but removing it here would still prevent some hassle (e.g., I had troubles configuring the build when absl is installed both with homebrew and conda: cmake was picking the wrong installation path).

Or is there anything (on the R side on or any other side) that forces us sticking with C++11 as the minimum supported standard? And/or is there other absl features that we'll need here, eventually?

paleolimbot commented 1 year ago

C++11 support is still a thing in R because the Windows toolchain used gcc 4.9 up to and including R 3.6, which will be on the support grid for tidyverse packages for another year or so.

I'm almost positive that you'll still need absl on the include path because absl::make_unique() is used in the S2 headers? Or maybe CMake takes care of that?

Somewhere inbetween would be to make some helpers (i.e., s2geography::internal::make_unique_polygon() or something)...I think most of the calls are for Polygon, Polyline, Loop, and the Geography subclasses and we don't need every single constructor overloaded.

benbovy commented 1 year ago

C++11 support is still a thing in R because the Windows toolchain used gcc 4.9 up to and including R 3.6, which will be on the support grid for tidyverse packages for another year or so.

Ok, let's keep using absl then, no worries.

I'm almost positive that you'll still need absl on the include path because absl::make_unique() is used in the S2 headers? Or maybe CMake takes care of that?

I guess ideally CMake should take care of that, but since there's no proper config cmake file for S2 installed with conda, brew, etc. it probably won't work in those cases.