Skip to content
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

Fixes #623 #624

Merged
merged 3 commits into from
May 10, 2017
Merged

Fixes #623 #624

merged 3 commits into from
May 10, 2017

Conversation

pmlopes
Copy link
Member

@pmlopes pmlopes commented May 9, 2017

Fixes #623

return Arrays.stream(COMMA_SPLITTER.split(unparsedHeaderValue))
// this is special case, since it's a single char and not a regex reserved one,
// it defaults to a simple fast lookup @see java.lang.String#split(java.lang.String)
return Arrays.stream(unparsedHeaderValue.split(","))
Copy link
Contributor

Choose a reason for hiding this comment

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

should use instead a precompiled regex, String#split is expensive

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 is a special case, if you look at the code of how String handles this you'll notice that it will not use regular expressions at all.

… to minimize restarts and object allocations
Copy link
Contributor

@alexlehm alexlehm left a comment

Choose a reason for hiding this comment

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

I wrote a few examples as test before I saw your new version and all pass with the new version except this one:

assertEquals("[\"a\"]", HeaderParser.convertToParsedHeaderValues("\\\"a\\\"", s -> s).toString());

I assume that the header does not make sense, it seems that the old version removed \" from anywhere in the string and the new one does it only for the values after =

If we can ignore this case, everything else is ok from my perspective

@pmlopes
Copy link
Member Author

pmlopes commented May 9, 2017

I was reading the RFC and quoted strings are only on values so I only check after the equals sign. I will read the RFC again to confirm this.

@pmlopes
Copy link
Member Author

pmlopes commented May 9, 2017

https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

Quoted strings are only after the equals sign.

@alexlehm
Copy link
Contributor

alexlehm commented May 9, 2017

ok, then the previous implementation was a bit more lenient, we can probably ignore that

@pmlopes pmlopes merged commit b5e9849 into master May 10, 2017
@pmlopes pmlopes removed the to review label May 10, 2017
@pmlopes pmlopes deleted the issues/623 branch May 31, 2017 07:45
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.

None yet

3 participants