DolphinTool: Sensible changes #11951
Conversation
| DiscIO::VolumeVerifier verifier(*volume, false, hashes_to_calculate); | ||
| verifier.Start(); | ||
| while (verifier.GetBytesProcessed() != verifier.GetTotalBytes()) |
There was a problem hiding this comment.
Why did you pull this out of a function? This feels worse to me than before.
There was a problem hiding this comment.
Do you want to avoid the copy of the DiscIO::VolumeVerifier::Result? In that case you forgot a &.
There was a problem hiding this comment.
I pulled it out of a function because it was wholly unnecessary for it to be outlined in the first place. For starters, it is only used once. It introduced an unnecessary failure state in the event that the DiscIO::VolumeDisc is a nullptr, which it never can be. Also, implementing the function inline took almost the same number of lines as it already took to call and check the result of VerifyCommand::VerifyVolume.
There was a problem hiding this comment.
And also this can avoid copying result, which I didn't know was returned by const reference until you just mentioned it.
There was a problem hiding this comment.
A function can be written simply for clarity even if it is only used once, or under the assumption that the functionality might be useful in another use case as well even if it doesn't exist at the moment.
Whatever, I think in this case it's not much of a difference either way, but your arguments feel a bit questionable to me.
Minty-Meeo
force-pushed
the
dolphin-tool-code-review-1
branch
from
8784d44
to
31e78c4
Compare
A subset of changes from #11943 which I believe to be most agreeable.