You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2023/04/08 19:48:14 UTC
[PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
ppkarwasz opened a new pull request, #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419
This PR fixes #1418 .
The buffer length in `Log4jFixedFormatter` was computed base on the length of the pattern. Now it is the double of the real length of a formatted date (in the English locale).
At the same time this migrates the `FastDateFormatter` tests to JUnit 5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419#discussion_r1162525131
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/datetime/FixedDateFormat.java:
##########
@@ -255,7 +261,7 @@ private static int[] nanoRange(final String pattern) {
* @return the length of the resulting formatted date and time strings
*/
public int getLength() {
- return pattern.length() - escapeCount;
+ return pattern.length() - escapeCount + extraTimeZoneFormatLength;
Review Comment:
This gives the exact length required in `Locale.ENGLISH`.
In other locales the length of short month names may be longer (e.g. it uses 3 Unicode characters beyond the BMP, so 6 chars). That's why I would use `2 * getLength()` as buffer length.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419#discussion_r1282997079
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/datetime/FixedDateFormat.java:
##########
@@ -503,6 +510,15 @@ public String getFormat() {
return fixedFormat.getPattern();
}
+ /**
+ * Returns the length of the resulting formatted date and time strings.
+ *
+ * @return the length of the resulting formatted date and time strings
Review Comment:
Right, I should add "in the ROOT locale" (which is basically English, but less anglocentric).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419#discussion_r1283038984
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/datetime/FixedDateFormat.java:
##########
@@ -503,6 +510,15 @@ public String getFormat() {
return fixedFormat.getPattern();
}
+ /**
+ * Returns the length of the resulting formatted date and time strings.
+ *
+ * @return the length of the resulting formatted date and time strings
Review Comment:
I will appreciate that. Maybe also a note on _"if you want to determine the max. length, you better multiply this number by 2"_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz merged PR #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419#discussion_r1283038984
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/datetime/FixedDateFormat.java:
##########
@@ -503,6 +510,15 @@ public String getFormat() {
return fixedFormat.getPattern();
}
+ /**
+ * Returns the length of the resulting formatted date and time strings.
+ *
+ * @return the length of the resulting formatted date and time strings
Review Comment:
I will appreciate that. Maybe also a note on _"if you want to determine the max. length independent of the used locale, you better multiply this number by 2"_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419#discussion_r1162488068
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/datetime/FixedDateFormat.java:
##########
@@ -255,7 +261,7 @@ private static int[] nanoRange(final String pattern) {
* @return the length of the resulting formatted date and time strings
*/
public int getLength() {
- return pattern.length() - escapeCount;
+ return pattern.length() - escapeCount + extraTimeZoneFormatLength;
Review Comment:
Given this is fixed, do we still need the `char[]`-size fix in `InstantFormatter`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Log4jFixedFormatter` buffer length (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1419:
URL: https://github.com/apache/logging-log4j2/pull/1419#discussion_r1282897067
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/datetime/FixedDateFormat.java:
##########
@@ -503,6 +510,15 @@ public String getFormat() {
return fixedFormat.getPattern();
}
+ /**
+ * Returns the length of the resulting formatted date and time strings.
+ *
+ * @return the length of the resulting formatted date and time strings
Review Comment:
This description is not correct, otherwise we won't need to double the size of the buffer in `InstantFormatter`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org