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: modernize cache.cc #3108

Merged
merged 13 commits into from Jun 12, 2022
Merged

Conversation

kvakvs
Copy link
Contributor

@kvakvs kvakvs commented May 18, 2022

  • Convert tr_cache to a class
  • Review size types and use size_t where appropriate
  • Replace C container for blocks with C++
    • Fix tests
  • Review and fix narrowing integer conversion warnings where possible
  • Add CLion temporary build directory to .clang-format-ignore and .gitignore

@kvakvs
Copy link
Contributor Author

kvakvs commented May 23, 2022

@ckerr What happened to the readonly array view (Span) merged last October in #1927 and removed by you the following week? Thought I could use it again to replace raw pointers pointing to raw data, and it's gone. For now i am going to use a reference to vector instead.

@ckerr
Copy link
Member

ckerr commented May 23, 2022

@ckerr What happened to the readonly array view (Span) merged last October in #1927 and removed by you the following week? Thought I could use it again to replace raw pointers pointing to raw data, and it's gone.

@kvakvs I removed it because I was going to use https://github.com/martinmoene/span-lite or https://github.com/microsoft/GSL ot https://github.com/gsl-lite/gsl-lite instead, but then I kept looping on which to use (mostly deciding between most-lightweight span-lite or best-maintained GSL) and never did anything.

If this is a blocker for this PR I'll take a look again. There are several places in libtransmission that would be improved by using a span.

@kvakvs
Copy link
Contributor Author

kvakvs commented May 23, 2022

For the moment it is not a blocker at all.
Worst that can happen, the code taking a pointer and a size remains at that, and no new checks or changes will be added.
But any function taking const any* or any* with a length is a direct candidate for a Span. Allows index out of range checking too, and allows to group size and pointer together forming a slice.

@kvakvs
Copy link
Contributor Author

kvakvs commented Jun 9, 2022

A few cache tests fail without actually crashing.
I could use some tips and hints which old trPtrArray operation is rewritten wrong, or at least which function of the new Cache is likely to be failing to make those tests fail.

@ckerr ckerr self-requested a review June 9, 2022 17:33
@ckerr
Copy link
Member

ckerr commented Jun 10, 2022

@kvakvs I'll try to take a look in the next couple of days. Sorry for the delay; a lot of my time this week has been taken up afk.

Also, FWIW needing help with this code is totally understandable. I kind of hate this code and I'm the one who wrote it 🤷‍♂️

@ckerr
Copy link
Member

ckerr commented Jun 10, 2022

@kvakvs kvakvs#1

@ckerr ckerr changed the title Modernize cache.cc refactor: modernize cache.cc Jun 11, 2022
@ckerr ckerr marked this pull request as ready for review June 11, 2022 23:06
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.

None yet

2 participants