Conversation
|
Can one of the admins verify this patch? |
|
@netty-bot test this please |
ejona86
left a comment
There was a problem hiding this comment.
These three changes look good and easy:
This one I think is currently a step backward:
- Simplify web of constructors and addComponents methods, reducing duplication of logic
Some addComponents0() do the checkNotNull, and others don't. addComponent0() does not consolidate but addComponents0() does. It makes the code more subtle and harder to audit.
That's not to say I'm against moving stuff around. But I feel like the current changes make it less consistent, not more.
There was a problem hiding this comment.
Seems this should be addComponent() (not 0).
There was a problem hiding this comment.
@ejona86 this wasn't changed in this PR, but I think it was intended to be the 0 version as written. The buffer b is certain to be non-null, and we're avoiding consolidation after each addition so that it can be done once at the end of the enclosing method.
There was a problem hiding this comment.
Oh, I missed the (b = it.next()) != null). I thought it was the normal b = it.next(). It doesn't seem the loop is any better with this change and I might argue it was more clear before. (It's not clear why it would choose to stop early if b == null, but it was obvious it was explicitly handling that case.)
|
I gave up trying to figure out if various addComponents() vs addComponents0() were the correct version. |
Motivation There's some miscellaneous cleanup/simplification of CompositeByteBuf which would help make the code a bit clearer. Modifications - Simplify web of constructors and addComponents methods, reducing duplication of logic - Rename `Component.freeIfNecessary()` method to just `free()`, which is less confusing (see netty#8641) - Make loop in addComponents0(...) method more verbose/readable (see netty#8437 (comment)) - Simplify addition/subtraction in setBytes(...) methods Result Smaller/clearer code
Now all those with 0-suffix don't include buffer null checks or consolidation, and those without _do_.
0710953 to
b1a485f
Compare
|
Thanks for trying to navigate this @ejona86! I was focused primarily on the de-duplication and you're right that I should have paid more attention to the consistency of the behaviour of the 0-suffix versions. In my defence I think most of the inconsistency was preexisting! I've added another small commit to make this consistent. Now none of the |
ejona86
left a comment
There was a problem hiding this comment.
Yeah, there was some minor weirdness with how addComponents0 would do checkNotNull, but that was minor (it wouldn't change the visible behavior) and it didn't seem any callers depended on the behavior. This most recent form has clear responsibilities again.
There was a problem hiding this comment.
Oh, I missed the (b = it.next()) != null). I thought it was the normal b = it.next(). It doesn't seem the loop is any better with this change and I might argue it was more clear before. (It's not clear why it would choose to stop early if b == null, but it was obvious it was explicitly handling that case.)
|
Thanks @ejona86. I've reverted that compaction now ... it was silly of me to have done it given one of the other changes in this same PR was to revert a similar occurrence! Supporting null as a terminator I think was the behaviour all along, maybe for consistency with the array cases where this could be helpful to avoid having to make a copy if populated with an unknown-upfront number of buffers. |
|
@netty-bot test this please |
|
@normanmaurer, I was happy with it already. Reverting the loop change was just icing on the cake. |
Motivation There's some miscellaneous cleanup/simplification of CompositeByteBuf which would help make the code a bit clearer. Modifications - Simplify web of constructors and addComponents methods, reducing duplication of logic - Rename `Component.freeIfNecessary()` method to just `free()`, which is less confusing (see #8641) - Make loop in addComponents0(...) method more verbose/readable (see #8437 (comment)) - Simplify addition/subtraction in setBytes(...) methods Result Smaller/clearer code
Motivation
There's some further miscellaneous cleanup/simplification of
CompositeByteBufwhich would help make the code a bit clearer.Modifications
addComponentsmethods, reducing duplication of logicComponent.freeIfNecessary()method to justfree(), which is less confusing (see CompositeByteBuf.Component freeIfNecessary always frees #8641)addComponents0(...)method more verbose/readable (see Streamline CompositeByteBuf internals #8437 (comment))setBytes(...)methodsResult
Smaller/clearer code