Limit unit magnitude + precision to fit into 63 bits. #16
Conversation
marshallpierce
force-pushed
the
better-fix-for-overflow-with-large-unit_magnitude
branch
5 times, most recently
from
2f66008 to
00f087b
Compare
src/tests/tests.rs
Outdated
| @@ -0,0 +1,14 @@ | |||
| use super::Histogram; | |||
|
|
|||
| #[path = "helpers.rs"] | |||
There was a problem hiding this comment.
Apparently the compiler is picky about this as of recently; I was surprised too. In future versions it will be a hard error, and right now it's a warning (which this project's setup makes an error anyway): rust-lang/rust#37872
There was a problem hiding this comment.
Oh wow, that's so strange. I know this is kind of annoying, but you could submit the #[path] changes in a separate PR (including the separation of helpers.rs) so that I have a way of referring back to that particular change? I'd then be happy to merge both that and this.
There was a problem hiding this comment.
OK. What sort of "referring back" do you need to do? It would be nice to avoid these git gymnastics; can you explain more about your workflow?
There was a problem hiding this comment.
In general, I like having one merge commit per distinct logical change, which translates into one PR per logical change. This makes it easier to find both where a change was introduced, and the conversation around why a change was introduced (by looking at the discussion in the corresponding PR). This also has the advantage that it more cleanly tracks the dependencies of a PR — we now know that the change in #18 is not necessary for #16, whereas the same would not be as easy to determine if 865a2e8 was just merged as a part of this PR.
There was a problem hiding this comment.
One example when this is really handy is when a bug is found and we need to bisect some day far into the future. blame also works a lot better when the merge that introduces the change is directly tied to the reason for the change.
There was a problem hiding this comment.
Hmmm... Having the same motivations, I come to the opposite conclusion. :( I want branches that represent a feature in its entirety, not a semi-arbitrary fragment of an overall feature. I would like to be able to locate the merge that introduced a bug, and then bisect along the branch that led to that merge, knowing that it has a good chance of navigating through every important change that's part of that feature. In what way is #18 not necessary for #16? It was merged before it, and #16 would not apply cleanly without it. And yet, the motivation for #18 is absent until you get to #16: the addition of another large block of tests.
IMO, developing an abstraction (of which splitting unit tests is a simple example) in a vacuum is a good way to get an abstraction that doesn't fit with the end goal. Or, going the other way, retroactively splitting it out is a good way to end up with a mangled branch -- on my first attempt, I didn't delete the old tests.rs, for instance.
Oh well. I guess this isn't likely to change your mind. We all come at this with different histories and biases.
There was a problem hiding this comment.
Interesting... I view the multiple commits that are a part of a single PR that way -- as the thing I would bisect further if the merge of that PR introduced a bug. But in #18 vs #16, if #18 introduced a bug, then clearly #16 was not at fault. And furthermore, #16 could be modified pretty easily to avoid the change in #18. The two changes are basically orthogonal.
I'm happy to not be as strict about this though. This is slowly but surely becoming as much your project as mine, and if that's how you prefer your merges, I'm okay with that 👍
There was a problem hiding this comment.
Thanks for being flexible! I shall also try to be slightly more granular than I normally would be in my PR's. ;)
This sort of stuff is all so path-dependent: once bitten twice shy, and where we've been bitten is very individual.
marshallpierce
force-pushed
the
better-fix-for-overflow-with-large-unit_magnitude
branch
from
00f087b to
10ac705
Compare
|
I merged #18. Can you rebase onto master, and then I'll merge this too. |
…sts ported from equivalent PR to Java impl.
marshallpierce
force-pushed
the
better-fix-for-overflow-with-large-unit_magnitude
branch
from
10ac705 to
e2891c7
Compare
|
Rebase is done. |
marshallpierce
deleted the
better-fix-for-overflow-with-large-unit_magnitude
branch
Approach and tests ported from equivalent PR to Java impl.
This addresses #14.
I split up the unit tests into multiple files based on the sort of thing they were testing. We're going to end up with a lot of tests and now seemed like a good time to start putting some structure around that.