feat: associative_vector rework
#41
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yes. There is no need to split and keep track of multiple PRs. Especially when all changes are related somehow and there are no competing PRs messing with the same code. |
|
About conventional commits, you can use "test" instead of "feat" for whatever is only related to tests.
These dicts/maps contain a more formal list of the commit types: https://github.com/alandefreitas/cpp-actions/blob/3507d049fa58363fd14aac8dfcf32732e54cc782/create-changelog/create-changelog.py#L87-L101 / https://github.com/alandefreitas/cpp-actions/blob/3507d049fa58363fd14aac8dfcf32732e54cc782/create-changelog/create-changelog.py#L862-L877 |
|
Right, I will reword my commit titles then. |
|
Man, I have no idea what clang-12 is on about... How is this not a constant expression? Meanwhile on my local machine with clang-14, there is a completely different error... And it also does not make sense. |
tearfur
force-pushed
the
erase-multi
branch
2 times, most recently
from
a7e1a99
to
185882f
Compare
It seems like you fixed it then. :) I've been using these https://github.com/alandefreitas/cpp-actions in libraries I maintain now (for instance, matplotplusplus and boost.url) but never got the time to update this library. We included many workarounds to fix conflicts we found between clang and updates in the runner images. Maybe there's some problem related to that. |
Err, if you can call "updating clang to a newer version that doesn't show this warning" fixing it, then yes. :)
|
hur dur UnREcHabLE CoDE
|
THERE we go. |
|
OK... I just tried the reworked fsanitize outputEdit: It happens also with c6960c7, the issue seems to be with the underlying vector. See #42 |
😆
Oh boy indeed. 😆 I have to take the time to implement the complete cpp-actions here. The way CI is set up does not scale well. So do you think this is good to go in the sense that it's better than before and whatever bugs we have were already there before? |
| if (it != end()) { | ||
| erase(it); | ||
| return 1; | ||
| return erase<decltype(k)>(k); |
There was a problem hiding this comment.
Isn't decltype(k) always the same? const key_type&?
There was a problem hiding this comment.
It is.
Maybe this is just a matter of taste, I think this is better than writing out the type twice.
|
Some files have changed enough that you add your name to the copyright :) |
Yes, I think this PR is good to go. I'm not aware of any more bugs within
Done, is this format ok? be11365 |
I think it should be something like // Copyright (c) 2024 Yat Ho ([email protected])
// Copyright (c) 2021 alandefreitas ([email protected]) |
Great. The way the commits are split also looks great. |
Done 3c06653 |
|
That's looking great. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #34
Fixes #35
Fixes #37
Fixes #39
Fixes #40
To make sure the changes are easy to trace, I was going to split this into 3 PRs, because I am used to squash merging. But since it looks like this repo is using rebase merge, I figured I can just lump all changes in to 1 PR.