Skip to content

Conversation

@leerho
Copy link
Member

@leerho leerho commented Jul 7, 2024

See comments associated with each file



=============================================================
Google Protobuf License:
Copy link
Member Author

Choose a reason for hiding this comment

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

Protobuf was used to test the UDF-8 code. Now that that has been removed, this license is no longer needed.


Code locations:
-------------------------------------------------------------
This product contains code for encoding, decoding and testing UTF8:
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a typo, that I just discovered. Zero-allocation-hashing is used for xxHash testing, not UTF8.

@@ -1,101 +1,102 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member Author

Choose a reason for hiding this comment

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

All format changes, except for the rename of one property.

@@ -1,22 +1,22 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member Author

Choose a reason for hiding this comment

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

Format changes, removed Protobuf dependency.

* @param byteOrder the ByteOrder to be used. It must be non-null.
* @return a new <i>Buffer</i> for read-only operations on the given ByteBuffer.
*/
static Buffer wrap(ByteBuffer byteBuffer, ByteOrder byteOrder) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Formatting: For all the public classes under ds/memory/ and for methods with more than one parameter, I have each parameter on a separate line. This is not only more readable, but comparing APIs across similar classes is much easier too.

*/
static Memory wrap(byte[] array, int offsetBytes, int lengthBytes, ByteOrder byteOrder) {
Objects.requireNonNull(array, "array must be non-null");
ResourceImpl.checkBounds(offsetBytes, lengthBytes, array.length);
Copy link
Member Author

Choose a reason for hiding this comment

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

These checks are done at the next call.

Objects.requireNonNull(byteOrder, "byteOrder must be non-null");
if (array instanceof byte[]) {
ResourceImpl.checkBounds(offsetBytes, lengthBytes, ((byte[])array).length);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a subtle bug that is only encountered if the user tries to use the ds/internal/BaseWritableBufferImpl directly -- which is out-of-scope.

int offsetBytes,
int lengthBytes,
ByteOrder byteOrder) {
return BaseWritableMemoryImpl.wrapHeapArray(array, offsetBytes, lengthBytes, true, byteOrder, null);
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a subtle bug that has been there since 2.0.0: The user-specified ByteOrder was overridden with ByteOrder.nativeOrder(). -- This would have affected the BO on subsequent multi-byte method calls only if the user specified a non-native BO.

@leerho leerho requested a review from jmalkin July 8, 2024 19:59
@leerho leerho merged commit 1b04c7a into master Jul 12, 2024
@leerho leerho deleted the prepare_for_3.0.0 branch July 12, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants