-
-
Notifications
You must be signed in to change notification settings - Fork 15.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ByteString introduced as AsciiString super class #3579
Conversation
protected EmbeddedChannel newContentCompressor(AsciiString contentEncoding) throws Http2Exception { | ||
if (GZIP.equalsIgnoreCase(contentEncoding) || X_GZIP.equalsIgnoreCase(contentEncoding)) { | ||
protected EmbeddedChannel newContentCompressor(ByteString contentEncoding) throws Http2Exception { | ||
if (GZIP.equals(contentEncoding) || X_GZIP.equals(contentEncoding)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this change. equalsIgnoreCase
doesn't fit well when dealing with binary strings. You have to think about encodings and translations...blah. HTTP/2 requires that header names are lower case. So a byte wise comparison should be sufficient, unless the user switches off the "force to lower case default"...in which case they are explicitly taking this risk.
@netkins build |
private static ByteStringProcessor VALIDATE_NAME_PROCESSOR = new ByteStringProcessor() { | ||
@Override | ||
public boolean process(byte value) throws Exception { | ||
// Check to see if the character is not an ASCII character |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing .
on EOL
6373547
to
afb4708
Compare
* {@link #array()}. Care must be taken when directly accessing the memory as that may invalidate assumptions that | ||
* this object is immutable. | ||
*/ | ||
public class ByteString { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, did you write this from scratch or is it a copy from another project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know it is based on the old AsciiString that was written by @trustin . He based it on code from Apache Harmony
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment, since you allow ByteString
to be extended, would it make sense to mark many of the frequently called methods as final
to help with optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanmaurer +1. This is mostly the result of pulling all the encoding neutral stuff from AsciiString
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmittler - Yip. I made a bunch of them final.
@Scottmitch looks great overall! Just a few questions/comments. |
@nmittler - Thanks for review! I think I got all your comments...feel free to take a look. |
Thanks @Scottmitch! Will review today. |
@trustin - Sounds good. Ping me when you are finished. |
@@ -706,83 +533,12 @@ public boolean equalsIgnoreCase(CharSequence string) { | |||
final byte[] value = this.value; | |||
final char[] buffer = new char[length]; | |||
for (int i = 0, j = start; i < length; i++, j++) { | |||
buffer[i] = (char) (value[j] & 0xFF); | |||
buffer[i] = (char) (value[j]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem. We should not drop & 0xFF
.
Those methods are just shortcut methods. We could move them to |
* Create a copy of the underlying storage from {@link value} into a byte array. | ||
* The copy will start at {@link ByteBuffer#position()} and copy {@link ByteBuffer#remaining()} bytes. | ||
*/ | ||
public static byte[] getBytes(ByteBuffer value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason these getBytes[]
methods should be public? They look unrelated to ByteString
and useful only for internal purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess they don't have to be. The thought was incase you don't want to have a ByteString
(which Ideally should be unmodifiable) object you could directly to byte[]
.
Review done. Please ping me for another round. Thank you for your patience! |
Good idea. I had to manually do some conversions which I was leaning on |
e9ec00b
to
920189b
Compare
* Create a copy of the underlying storage from {@link value} into a byte array. | ||
* The copy will start at {@link ByteBuffer#position()} and copy {@link ByteBuffer#remaining()} bytes. | ||
*/ | ||
public static byte[] getBytes(ByteBuffer value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trustin - Your comment was collapsed I think but the rational for having these public
is if a user wanted to directly get back a modifiable byte[]
. The "desired" use case for ByteString
is to be "unmodifiable". I can make private
though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make them private and consider moving them somewhere else and making them public in a later discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Am 14.04.2015 um 16:43 schrieb Scott Mitchell notifications@github.com:
In common/src/main/java/io/netty/util/ByteString.java:
- public ByteString(CharSequence value, Charset charset) {
this.value = getBytes(value, charset);
- }
- /**
\* @see {@link #getBytes(CharSequence, Charset, int, int)}
*/
- public ByteString(CharSequence value, Charset charset, int start, int length) {
this.value = getBytes(value, charset, start, length);
- }
- /**
\* Create a copy of the underlying storage from {@link value} into a byte array.
\* The copy will start at {@link ByteBuffer#position()} and copy {@link ByteBuffer#remaining()} bytes.
*/
- public static byte[] getBytes(ByteBuffer value) {
SGTM—
Reply to this email directly or view it on GitHub.
@trustin - Ready for another round. |
@Scottmitch Looks great! Please cherry-pick once you make the |
Motivation: The usage and code within AsciiString has exceeded the original design scope for this class. Its usage as a binary string is confusing and on the verge of violating interface assumptions in some spots. Modifications: - ByteString will be created as a base class to AsciiString. All of the generic byte handling processing will live in ByteString and all the special character encoding will live in AsciiString. Results: The AsciiString interface will be clarified. Users of AsciiString can now be clear of the limitations the class imposes while users of the ByteString class don't have to live with those limitations.
920189b
to
920c332
Compare
@Scottmitch thanks! Will update once I get to work! |
PR #3631 will serve to review the forward-port into master. |
Motivation: While forward porting #3579 there were a few areas that had not been previously back ported. Modifications: Backport the missed areas to ensure consistency. Result: More consistent 4.1 and master branches.
Motivation:
The usage and code within AsciiString has exceeded the original design scope for this class. Its usage as a binary string is confusing and on the verge of violating interface assumptions in some spots.
Modifications:
Results:
The AsciiString interface will be clarified. Users of AsciiString can now be clear of the limitations the class imposes while users of the ByteString class don't have to live with those limitations.