Update bn256 to c++20
#17
Conversation
src/array.h
Outdated
| } // namespace bn256 | ||
| #endif | ||
| template <typename T, std::size_t S> | ||
| using array = std::array<T, S>; |
There was a problem hiding this comment.
I think this should be removed and the code should be updated from array to std::array.
CMakeLists.txt
Outdated
| @@ -1,7 +1,7 @@ | |||
| cmake_minimum_required(VERSION 3.8) | |||
| project(bn256 VERSION 1.0.0) | |||
There was a problem hiding this comment.
I think it is worth updating the version on changing from C++-17 to C++20. Maybe 1.1.0. Would be good to create a release/1.0 branch with current code. I could see projects that are using C++-17 liking to have a C++17 version of the library available.
Note the README.md also needs to be updated with the correct version.
CMakeLists.txt
Outdated
| @@ -1,7 +1,7 @@ | |||
| cmake_minimum_required(VERSION 3.8) | |||
There was a problem hiding this comment.
needs to be updated to 3.12 to understand CMAKE_CXX_STANDARD=20
CMakeLists.txt
Outdated
| @@ -1,7 +1,7 @@ | |||
| cmake_minimum_required(VERSION 3.8) | |||
| cmake_minimum_required(VERSION 3.12) | |||
| project(bn256 VERSION 1.0.0) | |||
There was a problem hiding this comment.
Thanks, missed that, done!
CMakeLists.txt
Outdated
| string(REPLACE "." ";" version_list ${version}) # transform into list | ||
| list(GET version_list 0 version_major) | ||
|
|
||
| project(bn256 VERSION ${version}) |
There was a problem hiding this comment.
I think you can do project(bn256 VERSION 1.1.0) here and then use bn256_VERSION_MAJOR elsewhere, instead of doing that parsing stuff.
But it seems to me like this ought to be a 2.0.0 version instead, since don't these changes break the ABI?
There was a problem hiding this comment.
I think you can do project(bn256 VERSION 1.1.0) here and then use bn256_VERSION_MAJOR elsewhere,
yes, that works, thanks!
But it seems to me like this ought to be a 2.0.0 version instead
Hum, I'm not sure how to tell for sure, but I'm happy to switch to 2.0.0.
There was a problem hiding this comment.
I think it's mostly academic on 1.0.0 vs 1.1.0 vs 2.0.0 for many reasons.. but setting things like
Lines 23 to 25 in b20f7f5
shows an intent to version and since
std::span is part of the public interface, switching from "not really std::span" to std::span breaks usage of the library if it had been built as a shared library and installed. Again.. that seems like a stretch of a use case.. but maybe.
README.md
Outdated
| @@ -1,5 +1,5 @@ | |||
| # BN256 Cryptographic Library | |||
| ## Version : 1.0 | |||
| ## Version : 1.1 | |||
No description provided.