sipa / minisketch

Minisketch: an optimized library for BCH-based set reconciliation
MIT License
310 stars 52 forks source link

build: minor build changes #43

Closed fanquake closed 3 years ago

fanquake commented 3 years ago

This is very minor build changes, mostly updating external macros.

sipa commented 3 years ago

I'm not sure there is a reason for doing so, as long as we don't intend to use C++14 or C++17 features. Are there any issues mixing? I'd like to keep the requirements as low as possible, until there is a point.

gmaxwell commented 3 years ago

That was my thought. Incrementing the language version is costly-- reducing compatibility-- and should only be done if there are improvements that pay for it. Testing some future language version in CI to make sure there isn't any incompatibility that shows up-- seems fine to me.

sipa commented 3 years ago

Yes, doing one CI build that uses c++17 makes sense.

fanquake commented 3 years ago

Are there any issues mixing? I'd like to keep the requirements as low as possible, until there is a point.

One could be ABI compatibility issues, given that currently, the minisketch integration would result in the minisketch library being built with -std=c++11, while the rest of Core is built with -std=c++17 (I mistakenly thought that could also be the cause of the recently solved build issues in that PR.

However, after some discussion with @theuni, we're going to take a different approach to integrating the minisketch subtree (and will likely convert the other subtrees shortly after), utilizing the work that was done here in #8. That will mean that the minisketch build options / defaults etc, will no-longer be utilized by our build.

I've dropped the c++ default related change here.

sipa commented 3 years ago

@fanquake I'm not worried about ABI issues as the library has a C interface :)

sipa commented 3 years ago

utACK 7d3fe04d451033c25437fdf78ea8a7083a08ac98

sipa commented 3 years ago

Maybe update the PR description now, though?

fanquake commented 3 years ago

Maybe update the PR description now, though?

Whoops. Done.