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

refactor: use std::vector instead of bespoke Array class #7

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Jun 26, 2023

Part one of a three-part series to use std:: tools instead of bespoke ones. Part three will fix #6.

Unfortunately there don't seem to be any real tests for libutp, so the need to manually test is even greater than usual.

@ckerr ckerr requested a review from mikedld June 26, 2023 17:40
@ckerr
Copy link
Member Author

ckerr commented Jun 26, 2023

@tearfur also requesting review from you since you've been so active in the C++ code lately, but GitHub doesn't give me that option in the Reviewers list.

Possibly it only shows people who have been previously active in the repo 🤔 Along that same rationale, I'm not sure if @'ing you here will show up in your notifications. I guess if you don't pong here I'll ping you in another PR instead 😸

@mikedld
Copy link
Member

mikedld commented Jun 27, 2023

Unfortunately there don't seem to be any real tests for libutp, so the need to manually test is even greater than usual.

If we're forking it for good, might make sense to pull bittorrent#127 so that we're not completely blind here.

@ckerr
Copy link
Member Author

ckerr commented Jun 27, 2023

might make sense to pull bittorrent#127 so that we're not completely blind here

Agreed. @mikedld Can you copy libutp/pull/127 into a PR in this repo?

If we're forking it for good

The last upstream commit was six years ago; but even so, forking isn't my first choice.

Looks like the last maintainer comment in a PR was in 2019; however, it was by @arvidn who is still very active on libtorrent, so maybe there's hope. Arvid, do you have any thoughts on landing PRs in https://github.com/bittorrent/libutp?

@mikedld IMO even if we fork we should keep changes compatible w/upstream and try to upstream them, e.g. PRs like bittorrent#132 or this one, e.g. housekeeping & small fixes, not big changes or protocol changes.

@mikedld
Copy link
Member

mikedld commented Jun 30, 2023

It's already there in post-3.4-transmission branch, although uses CMake and not platform-specific build configs. Anyway, #8 is the slightly updated version of the upstream PR.

@mikedld
Copy link
Member

mikedld commented Jun 30, 2023

If it's not the fork-fork yet, did you mean to target post-3.4-transmission with this PR then? Otherwise, should probably merge post-3.4-transmission changes into master...

@ckerr ckerr force-pushed the refactor/replace-bespoke-to-std-pt-1 branch from 3495df1 to d9d3754 Compare June 30, 2023 20:24
@ckerr ckerr changed the base branch from master to post-3.4-transmission June 30, 2023 20:24
@ckerr
Copy link
Member Author

ckerr commented Jun 30, 2023

@mikedld, agreed. I've updated the branch to target post-3.4-transmission

@mikedld
Copy link
Member

mikedld commented Jul 1, 2023

@ckerr, CI seems to be unhappy with C++ standard now, need to adjust the default in

set(CMAKE_CXX_STANDARD 98)

@tearfur
Copy link
Member

tearfur commented Aug 24, 2023

@ckerr @mikedld ping

@mikedld
Copy link
Member

mikedld commented Aug 24, 2023

I'm fine with it, was under the impression that @ckerr is waiting out for something or maybe doing some more testing.

@ckerr
Copy link
Member Author

ckerr commented Oct 16, 2023

I guess I've been holding back on this PR because it really belongs upstream.

As mentioned above, there are a couple of followups to this. I hope all of them make it upstream someday -- I'm not going to make any big breaking changes.

@ckerr ckerr merged commit 2589200 into post-3.4-transmission Oct 16, 2023
10 checks passed
@ckerr ckerr deleted the refactor/replace-bespoke-to-std-pt-1 branch October 16, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Member access within misaligned address for type 'UTPSocketKeyData', which requires 8 byte alignment
3 participants