search

Tarsnap / tarsnap

Command-line client code for Tarsnap.
https://tarsnap.com
Other
864 stars 60 forks source link

Minor fixes #592

Closed dorjoy03 closed 10 months ago

dorjoy03 commented 10 months ago

Just a few smaller things I noticed.

dorjoy03 commented 10 months ago

I have now removed the SIZE_MAX changes

dorjoy03 commented 10 months ago

Noticed that in the resize function, there were expressions like EA->alloc * 2 and nsize * 4 which could overflow. Although not sure if it can ever happen in practice or the implications of it. I did some changes in e99ebbf to fix that.

gperciva commented 10 months ago

Looking good!

I'm still trying to wrap (no pun intended) my mind around the proposed changes to _append and _shrink, and whether anything should go into the header file, etc.

Could you please put the 2 commits for resize() into a new PR? (after modifying them)

That way, those 2 commits can move on to the next stage of review, without all the extra discussion in this PR. I'll continue to work on the perftests.

dorjoy03 commented 10 months ago

Thanks @gperciva

Could you please put the 2 commits for resize() into a new PR? (after modifying them)

I submitted a new PR https://github.com/Tarsnap/tarsnap/pull/595. I have gone through all the nitpicks there.

Should I reset this branch to the first commit and force push, now that the later 2 commits have been made into a separate PR?

gperciva commented 10 months ago

Thanks! When it's convenient, sure you could omit those commits from this PR. But it's not a priority right now, since there's still the perftests to go.

dorjoy03 commented 10 months ago

Okay. I will wait for the pertests then.

gperciva commented 10 months ago

When convenient, please omit those 2 commits, rebase on top of master, and do a force push.

Now that #597 is merged, I'm hoping that I'll have the ability to manually trigger the Actions.

dorjoy03 commented 10 months ago

When convenient, please omit those 2 commits, rebase on top of master, and do a force push.

Done.

gperciva commented 10 months ago

Hmm, unless I've done something terribly wrong in the perftest https://github.com/Tarsnap/libcperciva/pull/502, it looks like the changes are significantly slower. I'm surprised that there's this much of a difference: a factor of 2 slower for repeated _append() or _shrink() operations?

If you can see any mistakes in the perftest, I'm more than happy to fix them and run the tests again!

dorjoy03 commented 10 months ago

I believe there is nothing else to do other than to close this PR as per https://github.com/Tarsnap/libcperciva/pull/502#issuecomment-1780374002