Further tidying up of submitted PRs #367
Conversation
while `()` is a ZST the potentially dangling reference is still undefined behaviour. Making it a static is a trivial fix. Signed-off-by: Petros Angelatos <[email protected]>
The `TraceReader` trait uses associated types to define its `Key`, `Val`, `Time`, `Diff` but the `BatchReader` trait did not, even though they are very similar in nature. Usually the choice between asssociated types or generic parameters on a trait is determined by whether or not a particular type is expected to implement the same trait multiple times. My starting point was that these two trait should at the very least be consistent with respect to their structure and either both use generic parameters or both use associated types. All the uses in this repo (and also that I can imagine being useful) don't really need `BatchReader` to be polymorphic for a particular type and so I chose to change that one to make it consistent with `TraceReader`. The result is quite pleasing as in many cases a lot of generic parameters are erased. In order to keep this PR short I left the `Cursor` trait untouched, but I believe a similar transformation would be beneficial there too, simplifying further many type signatures. Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
while `()` is a ZST the potentially dangling reference is still undefined behaviour. Making it a static is a trivial fix. Signed-off-by: Petros Angelatos <[email protected]>
|
If things look confusing, it is because I started from just your PR descriptions rather than the PRs themselves. I knew I wanted to hit a few more things than you did (e.g. simplifying generic arguments to just the The manual |
petrosagg
force-pushed
the
tidy_up_take_two
branch
from
2ca878b
to
d4679be
Compare
|
Great, I dropped the last commit that added the manual |
|
Thanks much, and apologies for the confusion! |
@frankmcsherry thanks for reviewing #360 #361 and #363! I saw you incorporated it them all into #366 but the commits lost provenance/got mixed with other changes of yours.
I opened this PR that has a zero-diff with #366 but with the original commits preserved and the additional changes added by you on top. I looked at the additional changes and all looked good to me! The only question I had was regarding the manual
Cloneimplementations, which I've split out in the last commit of this PR. They look identical to what the automatically derived implementation would do, is it a stylistic preference or am I missing something?I'll close #360 #361 and #363 in hopes of getting this one merged 🤞