-
Notifications
You must be signed in to change notification settings - Fork 48
shared character toLowerCase function #809
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,13 @@ package org.apache.pekko.http.impl.model.parser | |
| import java.nio.charset.Charset | ||
|
|
||
| import org.apache.pekko | ||
| import org.parboiled2._ | ||
| import pekko.http.impl.util.{ enhanceString_, StringRendering } | ||
| import pekko.annotation.InternalApi | ||
| import pekko.http.impl.util.{ enhanceString_, CharUtils => CU, StringRendering } | ||
| import pekko.http.scaladsl.model.{ Uri, UriRendering } | ||
| import pekko.http.scaladsl.model.headers.HttpOrigin | ||
| import Parser.DeliveryScheme.Either | ||
| import Uri._ | ||
| import pekko.annotation.InternalApi | ||
| import org.parboiled2._ | ||
| import org.parboiled2.Parser.DeliveryScheme.Either | ||
|
|
||
| /** | ||
| * INTERNAL API | ||
|
|
@@ -365,7 +365,7 @@ private[http] final class UriParser( | |
|
|
||
| ///////////// helpers ///////////// | ||
|
|
||
| private def appendLowered(): Rule0 = rule { run(sb.append(CharUtils.toLowerCase(lastChar))) } | ||
| private def appendLowered(): Rule0 = rule { run(sb.append(CU.toLowerCase(lastChar))) } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we assume
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parboiled CharUtils only deals with the 26 standard letters of the latin alphabet (the ones in 7 bit ASCII table) - same as the pekko-http internal toLowerCase. |
||
|
|
||
| private def savePath() = rule { run(setPath(Path(sb.toString, uriParsingCharset))) } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * license agreements; and to You under the Apache License, version 2.0: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * This file is part of the Apache Pekko project, which was derived from Akka. | ||
| */ | ||
|
|
||
| /* | ||
| * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com> | ||
| */ | ||
|
|
||
| package org.apache.pekko.http.impl.util | ||
|
|
||
| import org.apache.pekko | ||
| import pekko.annotation.InternalApi | ||
|
|
||
| @InternalApi | ||
| private[http] object CharUtils { | ||
|
|
||
| /** | ||
| * Internal Pekko HTTP Use only. | ||
| * | ||
| * Efficiently lower-cases the given character. | ||
| * Note: only works for 7-bit ASCII letters (which is enough for header names) | ||
| */ | ||
| final def toLowerCase(c: Char): Char = | ||
| if (c >= 'A' && c <= 'Z') (c + 0x20 /* - 'A' + 'a' */ ).toChar else c | ||
|
|
||
| } |
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.
👍 clearly safe
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.
Actually, in hindsight, this might be the riskiest change. CharUtils.toLowerCase only lower cases the 26 standard letters of the latin alphabet (the ones in 7 bit ASCII table) while java.lang.Character.toLowerCase seems to support other letters that appear in Unicode table. Let me re-review this change.
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.
actually with (char1 | char2) < 0x80 in the check, I think we can assume that it doesn't matter that Character.toLowerCase can handle non-ASCII chars.