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

Modernize/bitfield.cc: Make into C++ #1917

Merged
merged 8 commits into from Oct 11, 2021

Conversation

kvakvs
Copy link
Contributor

@kvakvs kvakvs commented Oct 10, 2021

  • Renames: tr_bitfield becomes Bitfield, inner fields become private and snake_case_
  • All/None hints become an enum (this makes states of operation look a bit cleaner but please review)
  • Test module for bitfield also updated
  • Renamed some API functions: e.g. addsetBit, remclearBit, hasreadBit etc. Old naming was catering to the bitfield usage (as a storage of torrent blocks), new naming is neutral.

TODO:

  • getRaw - change from void* to owning pointer, update caller code
  • setFrom{Flags, Raw, ...} - change from pointer+size into a span which is essentially the same - a non-owning pair of pointer+size (note std::span arrives in C++20 so for C++17 we have to be creative)
  • C++ memory storage: change from byte* to a vector or unique_ptr

@ckerr
Copy link
Member

ckerr commented Oct 10, 2021

Hey @kvakvs, I love these patches and want to make sure I'm understanding right: the description has a TODO list, but also the PR is not marked as a draft, so I can't tell whether you're saying "this is a TODO of things left to be done in this PR" or "this is ready for review/merge and TODO is for a followup PR."

Either "this is a draft" or "this is ready" is fine; I just want to make sure I'm understanding right.

@ckerr ckerr added the needs clarification More info is needed before work can proceed label Oct 10, 2021
@kvakvs
Copy link
Contributor Author

kvakvs commented Oct 10, 2021

For this particular PR these changes are desired but they will probably explode the line count in the diff.
Should be a separate PR.

@kvakvs
Copy link
Contributor Author

kvakvs commented Oct 10, 2021

A loosely related question
I noticed the code uses uint64_t for time and milliseconds - there is a time unit support (std::chrono::milliseconds with uint64_t as storage type, which convert into other units if necessary and i can deliver that as a separate refactor). This can be carefully engineered to not leak C++ unit class into C header.
Also there are other units - bytes, and bits - for those there is no unit support in standard library (but there was something in Boost library which is not used here), but all I want here is to separate byte counts from bit counts, and wrap integer in a struct, making them essentially mutually incompatible types. Like so:

struct Bytes {
  uint64_t bytes;
  // and some arithmetic support + - > < etc
};
struct Bits {
  uint64_t bits;
  // and some arithmetic support + - > < etc
};

P.S. Clang is smart enough to treat these as simple uint64_t, not sure about GCC but we can also check.

@ckerr
Copy link
Member

ckerr commented Oct 10, 2021

At one point I was looking into using https://github.com/nholthaus/units in the Qt client; maybe it makes sense to look at it again for libtransmission as well. IIRC it's a header-only with no dependencies. If you're thinking about adding types for bits & bytes, maybe take a look at that and see what you think.

@kvakvs
Copy link
Contributor Author

kvakvs commented Oct 10, 2021

The library looks interesting, but i am confused which storage type is used for data and data_transfer_rate units. The base unit for both data size and data rate is byte and I hope it uses some sort of integer and not double like every other unit.
There seems to be a bit data type with ratio 8 to 1, nice.
I think for our case its enough to have simple wrappers with minimal amount of conversions.

@stefanos82
Copy link
Contributor

A loosely related question I noticed the code uses uint64_t for time and milliseconds - there is a time unit support (std::chrono::milliseconds with uint64_t as storage type, which convert into other units if necessary and i can deliver that as a separate refactor). This can be carefully engineered to not leak C++ unit class into C header. Also there are other units - bytes, and bits - for those there is no unit support in standard library (but there was something in Boost library which is not used here), but all I want here is to separate byte counts from bit counts, and wrap integer in a struct, making them essentially mutually incompatible types. Like so:

struct Bytes {
  uint64_t bytes;
  // and some arithmetic support + - > < etc
};
struct Bits {
  uint64_t bits;
  // and some arithmetic support + - > < etc
};

P.S. Clang is smart enough to treat these as simple uint64_t, not sure about GCC but we can also check.

How about use-defined literals, like how aria2 does it?

https://github.com/aria2/aria2/blob/faa6955c8d7bb7b6541e0b7fbb84f7d213f761ab/src/a2functional.h#L171-L186

@ckerr ckerr merged commit 39376f2 into transmission:master Oct 11, 2021
@ckerr ckerr added type:refactor A code change that neither fixes a bug nor adds a feature scope:core and removed needs clarification More info is needed before work can proceed labels Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:core type:refactor A code change that neither fixes a bug nor adds a feature
Development

Successfully merging this pull request may close these issues.

None yet

3 participants