Hi all, We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a')); Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong? If you're willing to take the change, I'd be happy to send a patch. Thanks, Eddie
No love from core-libs-dev? It's backwards-incompatible, but in a way that would unbreak existing broken code. Might be a worthwhile cleanup. Jeremy On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
I've encountered bugs in production code due to this surprise. A good thing to fix.
On Aug 16, 2014, at 1:38 AM, Jeremy Manson <jeremymanson@google.com> wrote:
No love from core-libs-dev? It's backwards-incompatible, but in a way that would unbreak existing broken code. Might be a worthwhile cleanup.
Jeremy
On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
There are indications this bug has actually occurred in released JDK code before (!!): http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b14... To give credit where credit is due, I reported the potential for this issue to Eddie after seeing it on StackOverflow at http://stackoverflow.com/q/25167015. On Aug 16, 2014 5:39 AM, "Andrew Thompson" <lordpixel@me.com> wrote:
I've encountered bugs in production code due to this surprise.
A good thing to fix.
On Aug 16, 2014, at 1:38 AM, Jeremy Manson <jeremymanson@google.com> wrote:
No love from core-libs-dev? It's backwards-incompatible, but in a way that would unbreak existing broken code. Might be a worthwhile cleanup.
Jeremy
On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
Yup, HG: changeset: 4823:194faa6fdb3c user: sherman date: Mon Dec 05 10:50:14 2011 -0800 files: src/share/classes/java/util/Formatter.java test/java/util/MissingFormatArgumentException/GetFormatSpecifier.java description: 5063455: (fmt) MissingFormatArgumentException.getFormatSpecifier() incorrect return value Summary: updated the incorrect StringBuilder constructor usage Reviewed-by: dholmes, sherman Contributed-by: brandon.passanisi@oracle.com JBS: https://bugs.openjdk.java.net/browse/JDK-5063455 -Pavel On 16 Aug 2014, at 20:17, Louis Wasserman <lowasser@google.com> wrote:
There are indications this bug has actually occurred in released JDK code before
I think the same problem also appears in StringBuffer and has been remarked on repeatedly over the years, e.g. http://osdir.com/ml/java.findbugs.general/2007-02/msg00068.html Michael Kay Saxonica mike@saxonica.com +44 (0) 118 946 5893 On 16 Aug 2014, at 20:17, Louis Wasserman <lowasser@google.com> wrote:
There are indications this bug has actually occurred in released JDK code before (!!):
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b14...
To give credit where credit is due, I reported the potential for this issue to Eddie after seeing it on StackOverflow at http://stackoverflow.com/q/25167015. On Aug 16, 2014 5:39 AM, "Andrew Thompson" <lordpixel@me.com> wrote:
I've encountered bugs in production code due to this surprise.
A good thing to fix.
On Aug 16, 2014, at 1:38 AM, Jeremy Manson <jeremymanson@google.com> wrote:
No love from core-libs-dev? It's backwards-incompatible, but in a way that would unbreak existing broken code. Might be a worthwhile cleanup.
Jeremy
On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
On Aug 15 2014, at 22:38 , Jeremy Manson <jeremymanson@google.com> wrote:
No love from core-libs-dev?
Pavel has been looking into this and doing corpus and historical bug checks. It seems possible that we might consider fixing it as it does seem to be an ongoing source of errors. Mike
It's backwards-incompatible, but in a way that would unbreak existing broken code. Might be a worthwhile cleanup.
Jeremy
On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
Is there a bug? Jeremy On Mon, Aug 18, 2014 at 12:02 PM, Mike Duigou <mike.duigou@oracle.com> wrote:
On Aug 15 2014, at 22:38 , Jeremy Manson <jeremymanson@google.com> wrote:
No love from core-libs-dev?
Pavel has been looking into this and doing corpus and historical bug checks. It seems possible that we might consider fixing it as it does seem to be an ongoing source of errors.
Mike
It's backwards-incompatible, but in a way that would unbreak existing broken code. Might be a worthwhile cleanup.
Jeremy
On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
As a general comment, in a new platform release like Java SE 9, it can be acceptable to add new overrides that would change the meaning of existing code: http://cr.openjdk.java.net/~darcy/OpenJdkDevGuide/OpenJdkDevelopersGuide.v0.... Truth be told, we've sometimes adding new methods which changes client code behavior accidentally, such as: JDK-6226858: NoSuchMethodError in BigDecimal when compiling with 1.5 targetted for 1.4 https://bugs.openjdk.java.net/browse/JDK-6226858 Cheers, -Joe On 08/18/2014 12:02 PM, Mike Duigou wrote:
On Aug 15 2014, at 22:38 , Jeremy Manson <jeremymanson@google.com> wrote:
No love from core-libs-dev? Pavel has been looking into this and doing corpus and historical bug checks. It seems possible that we might consider fixing it as it does seem to be an ongoing source of errors.
Mike
It's backwards-incompatible, but in a way that would unbreak existing broken code. Might be a worthwhile cleanup.
Jeremy
On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
So, should Eddie submit a patch? Someone on the Oracle side has to get it through the API gatekeepers. Jeremy On Mon, Aug 18, 2014 at 1:30 PM, Joe Darcy <joe.darcy@oracle.com> wrote:
As a general comment, in a new platform release like Java SE 9, it can be acceptable to add new overrides that would change the meaning of existing code:
http://cr.openjdk.java.net/~darcy/OpenJdkDevGuide/ OpenJdkDevelopersGuide.v0.777.html#general_evolution_policy
Truth be told, we've sometimes adding new methods which changes client code behavior accidentally, such as:
JDK-6226858: NoSuchMethodError in BigDecimal when compiling with 1.5 targetted for 1.4 https://bugs.openjdk.java.net/browse/JDK-6226858
Cheers,
-Joe
On 08/18/2014 12:02 PM, Mike Duigou wrote:
On Aug 15 2014, at 22:38 , Jeremy Manson <jeremymanson@google.com> wrote:
No love from core-libs-dev?
Pavel has been looking into this and doing corpus and historical bug checks. It seems possible that we might consider fixing it as it does seem to be an ongoing source of errors.
Mike
It's backwards-incompatible, but in a way that
would unbreak existing broken code. Might be a worthwhile cleanup.
Jeremy
On Fri, Aug 8, 2014 at 1:53 PM, Eddie Aftandilian <eaftan@google.com> wrote:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
Good catch ;-) -Ulf Am 08.08.2014 um 22:53 schrieb Eddie Aftandilian:
Additionally nice to have: (initial capacity with initial char(s)) StringBuilder(int,char) StringBuilder(int,CharSequence) -Ulf Am 08.08.2014 um 22:53 schrieb Eddie Aftandilian:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
On Aug 16 2014, at 16:24 , Ulf Zibis <Ulf.Zibis@cosoco.de> wrote:
Additionally nice to have: (initial capacity with initial char(s)) StringBuilder(int,char)
This one is unlikely.
StringBuilder(int,CharSequence)
I don't see much advantage to this API. If it enabled a significant optimization then it might be worth considering but I am inclined to doubt it's value. Mike
-Ulf
Am 08.08.2014 um 22:53 schrieb Eddie Aftandilian:
Hi all,
We recently realized that calling new StringBuilder(char) does not do what you would think it does. Since there is no char constructor defined, the char is widened to an int and the StringBuffer is presized to the character's encoded value. Thus code like this prints an empty string rather than the expected "a": System.out.println(new StringBuilder('a'));
Would it be possible to add a char constructor to StringBuilder to prevent this problem? I understand this would change the behavior of any code that is currently doing this, but it's hard to imagine anyone doing this intentionally. Of the ~20 instances we found in Google's codebase, all were bugs. What is your policy on making changes like this where (a) it will cause a change in behavior, but (b) the currently behavior is clearly wrong?
If you're willing to take the change, I'd be happy to send a patch.
Thanks, Eddie
participants (9)
-
Andrew Thompson
-
Eddie Aftandilian
-
Jeremy Manson
-
Joe Darcy
-
Louis Wasserman
-
Michael Kay
-
Mike Duigou
-
Pavel Rappo
-
Ulf Zibis