Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[FIX] Move remove_cvref_t into std namespace #2079

Merged
merged 4 commits into from
Sep 16, 2020
Merged

[FIX] Move remove_cvref_t into std namespace #2079

merged 4 commits into from
Sep 16, 2020

Conversation

wvdtoorn
Copy link
Contributor

@eseiler
Copy link
Member

eseiler commented Aug 29, 2020 •
edited
Loading

Hint for failing travis (click me):

std::remove_cvref_t is defined in cpp20 mode.
So you need to wrap the definition in a feature test macro, i.e.

#if feature_test_macro
template <typename t>
struct remove_cvref
...
#endif

You can look up specific macros here: https://en.cppreference.com/w/cpp/utility/feature_test
You can either search for the header they are defined in (type_traits) or search for the definition you are looking for (remove_cvref).

I think you can do the same with the type_identity that is defined in the same file while you are at it. It should have the same problem, but apparently we are not using it?

@marehr
Copy link
Member

marehr commented Aug 29, 2020

This should go against release;

Can you split the single commit into three commits?

  1. add remove_cvref to include/seqan3/std/type_traits
  2. change all occurrences to std::remove_cvref
  3. deprecate seqan3::remove_cvref (not removing it)

@codecov
Copy link

codecov bot commented Aug 30, 2020 •
edited
Loading

Codecov Report

Merging #2079 into release-3.0.2 will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release-3.0.2    #2079   +/-   ##
==============================================
  Coverage          97.82%   97.82%           
==============================================
  Files                264      264           
  Lines               9947     9947           
==============================================
  Hits                9731     9731           
  Misses               216      216
Impacted Files Coverage Δ
...e/seqan3/alignment/matrix/alignment_coordinate.hpp 100.00% <ø> (ø)
...n3/alignment/matrix/alignment_trace_algorithms.hpp 94.82% <ø> (ø)
include/seqan3/alignment/matrix/matrix_concept.hpp 100.00% <ø> (ø)
...clude/seqan3/alignment/pairwise/align_pairwise.hpp 100.00% <ø> (ø)
...ude/seqan3/alignment/pairwise/alignment_result.hpp 100.00% <ø> (ø)
...e/seqan3/alignment/pairwise/detail/type_traits.hpp 100.00% <ø> (ø)
...an3/alignment/pairwise/edit_distance_algorithm.hpp 100.00% <ø> (ø)
...qan3/alignment/pairwise/edit_distance_unbanded.hpp 99.25% <ø> (ø)
include/seqan3/alphabet/structure/concept.hpp 100.00% <ø> (ø)
include/seqan3/argument_parser/auxiliary.hpp 100.00% <ø> (ø)
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5eba128...6441c2a. Read the comment docs.

@wvdtoorn wvdtoorn force-pushed the remove_cvref_t-to-std-namespace branch from d571e68 to 56a0ab7 Compare August 30, 2020 09:47
@wvdtoorn wvdtoorn changed the base branch from master to release-3.0.2 August 30, 2020 09:47
@wvdtoorn wvdtoorn force-pushed the remove_cvref_t-to-std-namespace branch from 56a0ab7 to 43dfd4d Compare August 30, 2020 10:12
@wvdtoorn
Copy link
Contributor Author

Thank a lot both for the suggestions and help!

eseiler
eseiler reviewed Aug 30, 2020
View reviewed changes
include/seqan3/std/type_traits
Comment on lines +49 to +56
template <typename t>
using remove_cvref_t = typename remove_cvref<t>::type;
Copy link
Member

@eseiler eseiler Aug 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails on macOS GCC9, because std::remove_cvref_t is already defined.
Unfortunately, you can't put requires on typedefs (you can, but they are ignored)....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also fail on gcc9 linux...

Also, in line 21: #ifndef __cpp_lib_type_identity
should go down to line 56

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marehr That means we need a workaround macro? __cpp_lib_remove_cvref not being defined will be fixed in 9.4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marehr That means we need a workaround macro? __cpp_lib_remove_cvref not being defined will be fixed in 9.4

Do you know the bug ticket?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, this is the patch http://gcc.1065356.n8.nabble.com/committed-0-8-libstdc-Add-update-fix-feature-test-macros-td1682441.html

This is where I found it (remove_cvref has a note that says since 9.4 https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html)

eseiler
eseiler reviewed Aug 30, 2020
View reviewed changes
include/seqan3/alignment/matrix/detail/combined_score_and_trace_matrix.hpp Outdated
@@ -0,0 +1,354 @@
// -----------------------------------------------------------------------------------------------------
Copy link
Member

@eseiler eseiler Aug 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase error?

@marehr
Copy link
Member

marehr commented Sep 3, 2020

@wvandertoorn Is this still a draft?

@wvdtoorn
Copy link
Contributor Author

wvdtoorn commented Sep 3, 2020

@marehr Kinda, I was waiting on input to Erico's question on to how to handle the failing macro.

@marehr That means we need a workaround macro? __cpp_lib_remove_cvref not being defined will be fixed in 9.4

@wvdtoorn wvdtoorn force-pushed the remove_cvref_t-to-std-namespace branch from 43dfd4d to 32a5fd8 Compare September 8, 2020 12:32
@wvdtoorn wvdtoorn marked this pull request as ready for review September 8, 2020 12:33
@marehr marehr force-pushed the remove_cvref_t-to-std-namespace branch from fff7164 to eab44f4 Compare September 16, 2020 10:07
@marehr marehr requested a review from eseiler September 16, 2020 14:00
@marehr
Copy link
Member

marehr commented Sep 16, 2020

@eseiler Can you have a look? I think I got it fixed :)

@marehr marehr force-pushed the remove_cvref_t-to-std-namespace branch from c382781 to ffa1a27 Compare September 16, 2020 14:17
@marehr marehr force-pushed the remove_cvref_t-to-std-namespace branch from ffa1a27 to 4e1d3fb Compare September 16, 2020 14:20
@marehr marehr force-pushed the remove_cvref_t-to-std-namespace branch from 4e1d3fb to 6441c2a Compare September 16, 2020 14:22
@marehr marehr self-requested a review September 16, 2020 16:22
@eseiler eseiler merged commit 3bee0fd into seqan:release-3.0.2 Sep 16, 2020
@marehr marehr mentioned this pull request Sep 18, 2020
Merged
@marehr marehr mentioned this pull request Oct 14, 2020
Closed
2 tasks
@marehr marehr mentioned this pull request Mar 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eseiler eseiler eseiler approved these changes

@marehr marehr marehr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move C++20 things into std namespace
3 participants
@wvdtoorn @eseiler @marehr