Skip to content

Conversation

@georgemavchun
Copy link

Few bug fixes included.
In each commit there is test that fails without fix commit and doesn't fail with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the write buffer size that was passed in?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because bufferSize >= lengthLength + objectSize must be true in all cases.
If it is not so, user has entered incorrect values. We can react on that two ways:
first - throw an exception, second - use max(bufferSize, lengthLength + objectSize).
We have chosen second one, but we can change to exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I prefer an exception to using a buffer size other than what was specified.

@NathanSweet
Copy link
Member

Combining multiple fixes in the same PR makes review difficult. Please explain the fixes.

@georgemavchun
Copy link
Author

Should we delete that PR and resubmit few new? Or comments we have written is enough?

@georgemavchun
Copy link
Author

So what should we do now?

@jgandert
Copy link

jgandert commented Jul 7, 2016

@NathanSweet Any news on this?

@NathanSweet
Copy link
Member

Splitting it into multiple PRs would help a lot.

ai-dsxt added 2 commits June 19, 2019 12:21
Dsx 2: Check classes registered in the client and the server are the same
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants