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.

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

[INFRA] Drop gcc9 #2952

Merged
merged 22 commits into from
Mar 31, 2022
Merged

[INFRA] Drop gcc9 #2952

merged 22 commits into from
Mar 31, 2022

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Mar 25, 2022

No description provided.

@vercel
Copy link

vercel bot commented Mar 25, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/3qZfjUW2qxe5pYjdwmcygai3sxTV
✅ Preview: https://seqan3-git-fork-eseiler-infra-dropgcc9-seqan.vercel.app

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #2952 (3049495) into master (576322c) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 3049495 differs from pull request most recent head d081e66. Consider uploading reports for the commit d081e66 to get more accurate results

@@           Coverage Diff           @@
##           master    #2952   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files         267      267           
  Lines       11494    11494           
=======================================
  Hits        11290    11290           
  Misses        204      204           
Impacted Files Coverage Δ
...ment/aligned_sequence/aligned_sequence_concept.hpp 100.00% <ø> (ø)
...ment/configuration/align_config_scoring_scheme.hpp 100.00% <ø> (ø)
...clude/seqan3/alignment/decorator/gap_decorator.hpp 100.00% <ø> (ø)
...matrix/detail/advanceable_alignment_coordinate.hpp 100.00% <ø> (ø)
...qan3/alignment/matrix/detail/affine_cell_proxy.hpp 100.00% <ø> (ø)
...ignment/matrix/detail/aligned_sequence_builder.hpp 100.00% <ø> (ø)
...etail/alignment_matrix_column_major_range_base.hpp 100.00% <ø> (ø)
...qan3/alignment/matrix/detail/alignment_optimum.hpp 100.00% <ø> (ø)
...atrix/detail/alignment_score_matrix_one_column.hpp 100.00% <ø> (ø)
...etail/alignment_score_matrix_one_column_banded.hpp 97.43% <ø> (ø)
... and 101 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 576322c...d081e66. Read the comment docs.

@eseiler eseiler force-pushed the infra/drop_gcc9 branch 2 times, most recently from 1058327 to e0b6ef2 Compare March 25, 2022 20:56
Copy link
Contributor

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

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

A lot of changes. My only real remark is my last annotation. Something about a shim-implementation for floating types. I didn't get that part :/

The rest looks good.

I also added a few remarks here and there, that arn't really part of the PR, but I would also never look at it again and wasn't sure if it is correct.

f = range;
#endif
return std::move(f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is return f not enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷
It is definitely needed, because I tried to remove it :D
Copy elision doesn't take effect here.
I think this is some code where we have overloads for each const & && combo?

{insert_gap(v, std::ranges::begin(v), 2)} -> std::same_as<std::ranges::iterator_t<t>>;
{erase_gap(v, std::ranges::begin(v))} -> std::same_as<std::ranges::iterator_t<t>>;
{erase_gap(v, std::ranges::begin(v), std::ranges::end(v))} -> std::same_as<std::ranges::iterator_t<t>>;
{assign_unaligned(v, unaligned)} -> std::same_as<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks so much nicer now!

@@ -230,7 +230,7 @@ class inherited_iterator_base : public std::conditional_t<std::is_pointer_v<base
constexpr derived_t operator++(int) noexcept(noexcept(std::declval<base_t &>()++) &&
noexcept(derived_t(std::declval<base_t &>())))
//!\cond
requires requires (base_t_ i) { i++; SEQAN3_RETURN_TYPE_CONSTRAINT(i++, std::same_as, base_t_); } &&
requires requires (base_t_ i) { i++; {i++} -> std::same_as<base_t_>; } &&
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we leave the first i++ away?
requires requires (base_t_ i) { {i++} -> std::same_as<base_t_>; } &&

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but I wouldn't change the concept in this PR.
Theoretically, it first checks whether i++ is valid, and then whether it returns the correct type. The first i++ is probably not needed.

SEQAN3_RETURN_TYPE_CONSTRAINT(std::swap(val, val2), std::same_as, void);
{val.swap(val2)} -> std::same_as<void>;
{swap(val, val2)} -> std::same_as<void>;
{std::swap(val, val2)} -> std::same_as<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find where it says that std::swap has to be possible by the standard.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I hope these concepts are in detail? They are not really usable as one usually only needs a container to be e.g. "push-backable" but not everything else. I think this was already decided but never done

@@ -66,7 +66,7 @@ Please see the [online documentation](https://docs.seqan.de/seqan/3-master-user/

| | requirement | version | comment |
|-------------------|------------------------------------------------------|----------|---------------------------------------------|
|**compiler** | [GCC](https://gcc.gnu.org) | ≥ 7 | no other compiler is currently supported! |
|**compiler** | [GCC](https://gcc.gnu.org) | ≥ 10 | no other compiler is currently supported! |
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

#include <utility> // __cpp_lib_to_chars may be defined here as currently documented
#include <charconv>
#include <utility> // __cpp_lib_to_chars may be defined here as currently documented.
#include <version> // From C++20 onwards, all feature macros should be defined here.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment after the include is not required anymore.


// =========================================================================
// If any std implementation is present use that as basis (>= gcc8)
// If float implementation is missing, add our own shim-implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should still be any std?

Copy link
Member Author

Choose a reason for hiding this comment

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

int is provided for gcc >= 10, float for >= 11, I think.
I removed the int shim, so we only ever add float

@eseiler eseiler requested a review from SGSSGene March 30, 2022 14:50
Copy link
Contributor

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

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

Lets go and drop gcc9!!!!

@eseiler eseiler requested a review from smehringer March 31, 2022 07:56
@smehringer smehringer merged commit dc1ef8c into seqan:master Mar 31, 2022
@eseiler eseiler deleted the infra/drop_gcc9 branch March 31, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants