-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add GCC9 support, update range-v3 to 1.0-beta #708
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor questions
{ | ||
#else // GCC/Clang extension | ||
#pragma GCC diagnostic push | ||
#pragma GCC diagnostic ignored "-Wpedantic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the warning we're ignoring here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below is an extension in GCC/Clang, GCC9 now warns in pedantic mode that this extension is not canonical c++ and never made it into C++20.
/*!\typedef std::ranges::default_sentinel_t | ||
* \brief Alias for ranges::default_sentinel_t. Type of ranges::default_sentinel. | ||
*/ | ||
using SEQAN3_DOXYGEN_ONLY(default_sentinel_t =) ::ranges::default_sentinel_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these changes rather belong to commit 40e7c35 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no, I like to keep the file changes together and I think "adapting std" is a good logical unit and changes in our library are then different.
|
||
//!\cond | ||
template <typename t> | ||
requires true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o.0 huh.. what hacky workaround is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in presence of a previous definition these lines appear as a specialisation, but in the absence of a previous definition, these appear as the definition.
test/header/CMakeLists.txt
Outdated
@@ -103,7 +103,9 @@ seqan3_require_test () | |||
|
|||
seqan3_header_test (seqan3 "${SEQAN3_CLONE_DIR}/include") | |||
seqan3_header_test (seqan3_test "${SEQAN3_CLONE_DIR}/test/include") | |||
seqan3_header_test (range-v3 "${SEQAN3_CLONE_DIR}/submodules/range-v3/include") | |||
|
|||
# contains deprecated that error on inlusion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍺
Codecov Report
@@ Coverage Diff @@
## master #708 +/- ##
==========================================
+ Coverage 94.09% 94.13% +0.03%
==========================================
Files 152 152
Lines 4881 4861 -20
==========================================
- Hits 4593 4576 -17
+ Misses 288 285 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Please review, but do not merge, yet. I recommend reviewing commit-by-commit.
We need to have the blockers fixed, before we can merge this (everything builds locally with applied patches in range-v3 and sdsl).
Blockers (GCC7 and GCC8):
make semiregular copy constructor be conditionally noexcept ericniebler/range-v3#1023[FIX] minus operator wasn't const qualified xxsds/sdsl-lite#57further additions of noexcept to int_vector xxsds/sdsl-lite#58Blockers (GCC9, fixes available in next snapshot):
v1.0-beta ICEs with GCC9 ericniebler/range-v3#1011https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89089https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89117