leerho commented on issue #502:
URL:
https://github.com/apache/datasketches-cpp/issues/502#issuecomment-5723704073
We have several repos as a part of the DataSketches project that depend on
our current datasketches-cpp repo.
I asked Claude to analyze the impact of moving this repo to C++ 17 for each
one:
## datasketches-postgresql
* **It is Straightforward to update the repo.** It's -std=c++17 in two
places (Makefile:75 and the bitcode rule at Makefile:85), plus doc/packaging
bookkeeping. The code itself already compiles clean at C++17 and C++20. The
bitcode rule is the one that's easy to forget, and the only thing that would
produce a non-obvious failure (JIT-enabled PG builds with an older clang).
* **There is no impact on the interaction with PostgreSQL itself.** The
server never sees C++; the boundary is a C ABI and the extension already keeps
PG headers out of the C++ TUs. Stored sketches, error handling, and memory
management are all unaffected by the standard bump.
* **Environment change.**
The only caveat that's a real-world consideration rather than a code change:
users must build and run on a toolchain/libstdc++ new enough for C++17 (GCC ≥
7/8, clang ≥ 5), which effectively drops RHEL 7-era systems. That's an
environment requirement, not a PostgreSQL compatibility issue.
## datasketches-python
This repo already builds at C++17 (set(CMAKE_CXX_STANDARD 17) in
CMakeLists.txt:39).
Short answer: essentially none. This repo is already a C++17 project, and it
consumes datasketches-cpp in a way that is insulated from the C++ library's own
standard setting.
Why there's no impact
1. The Python bindings already compile at C++17.
CMakeLists.txt:39 sets set(CMAKE_CXX_STANDARD 17). This came in with the
nanobind conversion (commit 9ab84cc), because nanobind itself requires C++17.
The wrappers even use C++17 library features directly — e.g. std::variant in
src/vector_of_kll.cpp:69. So every datasketches-cpp header this repo includes
is already being compiled as C++17, today, with the 5.2.0 pin.
2. datasketches-cpp's own CMake settings never reach this build.
The C++ library is pulled in via ExternalProject_Add
(CMakeLists.txt:117-125) and then only its installed header directory is added
to the include path (CMakeLists.txt:130-133). This repo never links the
datasketches CMake target, so:
- set(CMAKE_CXX_STANDARD 11) in the C++ repo only governs its own test
executables, which are disabled here with -DBUILD_TESTS=OFF.
- target_compile_features(datasketches INTERFACE cxx_std_11) is an
interface property on a target this project doesn't consume. If it became
cxx_std_17, nothing here would change.
3. Toolchains are already C++17-capable.
- ci.yml: macos-latest (Clang), ubuntu-latest (GCC), windows-latest
(MSVC 2019) — all fine.
- build_wheels.yml already sets MACOSX_DEPLOYMENT_TARGET: "10.14" # min
supporting c++17, and the manylinux/musllinux cibuildwheel images ship modern
GCC.
- Source-dist users already need a C++17 compiler because of nanobind,
so no new requirement is imposed on end users.
4. The version pin buffers everything anyway.
GIT_TAG 5.2.0 means the Python repo won't see any C++ change — standard bump
or otherwise — until someone deliberately bumps the tag.
What could require work (but isn't caused by the standard bump itself)
The only real exposure is API changes that ride along with the upgrade. If
the C++ library used the move to 17 as an opportunity to, say, replace
hand-rolled types with std::optional/std::string_view, change allocator/serde
template signatures, or drop C++11-era workarounds, then the wrapper files in
src/*.cpp and the local adapter headers in include/ (py_serde.hpp,
py_object_lt.hpp, py_object_ostream.hpp, quantile_conditional.hpp,
kernel_function.hpp, tuple_policy.hpp) would need to track those changes when
the GIT_TAG is bumped. That's ordinary dependency-upgrade maintenance, not a
consequence of C++17 per se.
One minor upside: once the C++ library is officially C++17, the Python
wrappers could freely rely on C++17 in shared code without worrying about
diverging from what the upstream headers assume — though as noted, they already
do in practice.
## datasketches-bigquery
Short answer: essentially no impact. This repo is already compiling
datasketches-cpp in C++17 mode today, and it doesn't consume anything a C++17
upgrade would plausibly remove.
Here's the evidence, from the repo as it sits on disk:
Why the impact is ~zero
1. The build never sees datasketches-cpp's CMake standard setting.
*datasketches-cpp/CMakeLists.txt:46* sets *CMAKE_CXX_STANDARD 11* and line
107 declares *cxx_std_11,* but this repo doesn't use CMake at all. Each
module's Makefile (e.g. *theta/Makefile:19-29*) just passes
*-I../datasketches-cpp/common/include -I../datasketches-cpp/<module>/include*
to emcc. Whatever standard datasketches-cpp declares is irrelevant here — only
what the headers require matters.
2. *emcc* already defaults to C++17.
No Makefile passes a *-std=* flag, so the default applies. With the locally
installed Emscripten 4.0.10:
```bash
$ echo | emcc -dM -E -x c++ - | grep __cplusplus
\#define __cplusplus 201703L
```
Clang has defaulted to *gnu++17* since Clang 16, and *cloudbuild.yaml* uses
*emsdk install latest* (6.0.1 per the recent commits), so CI is on the same or
newer default. Every WASM artifact you've shipped since the emscripten upgrade
was already built from these headers under C++17.
3. The wrapper code doesn't touch the C++11 compatibility shims.
The most likely breaking change in a C++17 modernization would be deleting
the hand-rolled shims in *common/include/ — optional.hpp* (replaced by
*std::optional*), *conditional_forward.hpp*, *conditional_back_inserter.hpp*,
etc. I grepped all 837 lines of wrapper *.cpp* across the eight modules: they
include only the public sketch headers (*theta_sketch.hpp*, *kll_sketch.hpp*,
*hll.hpp*, …) and reference only public types (*datasketches::kll_sketch*,
*datasketches::theta_union*, *DEFAULT_SEED*, constants). Zero references to
*datasketches::optional*, serde internals, or any *common/* header.
4. Nothing C++17 removed is used here.
No *auto_ptr*, *random_shuffle,* *unary_function/binary_function*,
*bind1st/2nd*, *register*, dynamic exception specs, or trigraphs in the wrapper
code.
5. Verified by compiling. All eight wrappers pass *emcc -fsyntax-only
-std=c++17 -Wall -Wextra* against the vendored 5.1.0 headers with no errors.
The only warnings are pre-existing and orthogonal to the standard (two
*-Wunnecessary-virtual-specifier* inside datasketches-cpp's HLL headers, plus
one noted below).
Things worth knowing anyway
* **The vendored checkout is stale vs. the Makefile.** *datasketches-cpp/*
on disk is a 5.1.0 checkout (*version.cfg.in*), while *Makefile:27* downloads
5.2.0. Irrelevant to the C++17 question, but if a future release changes any
public API (renamed method, changed constructor signature), that — not the
standard bump — is what would break the wrappers, and you'd catch it by bumping
*DATASKETCHES_CPP_VERSION* and running *make*.
* One genuine deprecation in this repo's own code, unrelated to C++17:
*theta/theta_sketch.cpp:97* uses std::strstream (also *#included* but unused in
*cpc/cpc_sketch.cpp:20* and *hll/hll_sketch.cpp:20*). It's been deprecated
since C++98 and is removed in C++26, so it will become a hard error once
emscripten's libc++ defaults there. It's a small fix — *std::istringstream*
over a *std::string*, or better, datasketches' *deserialize(const void* bytes,
size_t size)* overload which avoids the stream entirely — but it's not urgent
and it's not triggered by a C++17 move.
* Optional hardening: if you want the build to be explicit rather than
relying on clang's default, add *-std=c++17* to each module's *EMCFLAGS*. Given
the verification above it's a no-op today, so it's purely documentary.
One thing I can't evaluate from here is whether the datasketches-cpp upgrade
would also change public API surface as part of the same release (e.g.
returning *std::optional* from something that currently throws on empty).
That's a release-notes question, not a standard-version question — the C++17
part itself is a non-event for this repo.
## datasketches-cuda
This is a brand new repo and my conversation with the developers is that
moving to C++ 17 is a non-event, but nonetheless a welcome one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
