Re: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt content when size of element is > 8192

2016-12-02 Thread Daniel Fuchs 

On 01/12/16 17:25, Joe Wang wrote:

Thanks Christoph, Daniel! I've updated the test, removing the comments.

As for 80 chars line limit, or slightly longer than that is fine with
me. Given our codebase, it would be an enormous undertaking. I'm
therefore fine with taking care of only the extremely long ones or any
that impedes the reviews, that is, side-by-side view.


+1

best regards,

-- daniel



Best,
Joe

On 12/1/16, 2:25 AM, Daniel Fuchs wrote:

Hi Joe,

I agree with Christoph's comments below.

+1

best regards,

-- daniel

On 01/12/16 07:40, Langer, Christoph wrote:

Hi Joe,

to me this looks good.

Did you already remove the cleanups from
http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ ? I can't see
a lot of them any more...

A few minor points:

It seems you still have left some debugging code in
test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest.java:


59 while (xmlStreamReader.hasNext()) {
  60 int event = xmlStreamReader.next();
  61 if (event == XMLStreamConstants.START_ELEMENT) {
  62 if
(xmlStreamReader.getLocalName().equals("body"))// && bMessage)

-> remove the && bMessage)

  63 {
  64 String elementText =
xmlStreamReader.getElementText();
  65 //System.out.println("elementText=" +
elementText + "EndOfElementText");

-> the commented System.out.println statement should be removed at
all, I suggest

  66 // fail if elementText contains ""
  67
Assert.assertTrue(!elementText.contains(""), "Fail:
elementText contains ");
  68 }
  69 }
  70 }

Other than that I'm wondering if the 80 chars line limit shall be
held which is not completely true in StreamReaderTest.java. But I
know how difficult that is, while keeping the code still readable.
Especially with the long speaking Class and method names ;-)

Best regards
Christoph



-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net]
On Behalf
Of Joe Wang
Sent: Mittwoch, 30. November 2016 22:21
To: core-libs-dev@openjdk.java.net
Subject: RFR (JAXP) 8167340: XMLStreamReader.getElementText return
corrupt
content when size of element is > 8192

Hi,

Please review an one-line fix and a bunch of cleanups.

The reported issue was caused by a missed setting when the
XMLStreamReader initializes the XML 1.1 scanner, so while the changeset
involved 350 lines, the fix is really just the following:

XMLStreamReaderImpl.java:
@@ -605,11 +604,12 @@
...
+fEntityScanner.registerListener(fScanner);


All other changes are cleanups, warnings. And BTW, warnings in the jaxp
repo have come down from 5230 in 2015 to 3265, a result of a bit of
cleanups here and there when we touch those classes. Still a long
way to
go, and it shows we may need to have a few dedicated patches.

JBS: https://bugs.openjdk.java.net/browse/JDK-8167340
webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/

Thanks,
Joe

RE: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt content when size of element is > 8192

2016-12-02 Thread Langer, Christoph 
Hi Joe,

thanks, looks fine now.

In 
test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest.java, 
the brace in line 63 could still move one line up, but this really is 
nit-picking ;-)

Best regards
Christoph


> -Original Message-
> From: Joe Wang [mailto:huizhe.w...@oracle.com]
> Sent: Donnerstag, 1. Dezember 2016 18:25
> To: Daniel Fuchs <daniel.fu...@oracle.com>
> Cc: Langer, Christoph <christoph.lan...@sap.com>; core-libs-
> d...@openjdk.java.net
> Subject: Re: RFR (JAXP) 8167340: XMLStreamReader.getElementText return
> corrupt content when size of element is > 8192
> 
> Thanks Christoph, Daniel! I've updated the test, removing the comments.
> 
> As for 80 chars line limit, or slightly longer than that is fine with
> me. Given our codebase, it would be an enormous undertaking. I'm
> therefore fine with taking care of only the extremely long ones or any
> that impedes the reviews, that is, side-by-side view.
> 
> Best,
> Joe
> 
> On 12/1/16, 2:25 AM, Daniel Fuchs wrote:
> > Hi Joe,
> >
> > I agree with Christoph's comments below.
> >
> > +1
> >
> > best regards,
> >
> > -- daniel
> >
> > On 01/12/16 07:40, Langer, Christoph wrote:
> >> Hi Joe,
> >>
> >> to me this looks good.
> >>
> >> Did you already remove the cleanups from
> >> http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ ? I can't see
> >> a lot of them any more...
> >>
> >> A few minor points:
> >>
> >> It seems you still have left some debugging code in
> >>
> test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest
> .java:
> >>
> >> 59 while (xmlStreamReader.hasNext()) {
> >>   60 int event = xmlStreamReader.next();
> >>   61 if (event == XMLStreamConstants.START_ELEMENT) {
> >>   62 if
> >> (xmlStreamReader.getLocalName().equals("body"))// && bMessage)
> >>
> >> -> remove the && bMessage)
> >>
> >>   63 {
> >>   64 String elementText =
> >> xmlStreamReader.getElementText();
> >>   65 //System.out.println("elementText=" +
> >> elementText + "EndOfElementText");
> >>
> >> -> the commented System.out.println statement should be removed at
> >> all, I suggest
> >>
> >>   66 // fail if elementText contains ""
> >>   67
> >> Assert.assertTrue(!elementText.contains(""), "Fail:
> >> elementText contains ");
> >>   68 }
> >>   69 }
> >>   70 }
> >>
> >> Other than that I'm wondering if the 80 chars line limit shall be
> >> held which is not completely true in StreamReaderTest.java. But I
> >> know how difficult that is, while keeping the code still readable.
> >> Especially with the long speaking Class and method names ;-)
> >>
> >> Best regards
> >> Christoph
> >>
> >>
> >>> -Original Message-
> >>> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net]
> >>> On Behalf
> >>> Of Joe Wang
> >>> Sent: Mittwoch, 30. November 2016 22:21
> >>> To: core-libs-dev@openjdk.java.net
> >>> Subject: RFR (JAXP) 8167340: XMLStreamReader.getElementText return
> >>> corrupt
> >>> content when size of element is > 8192
> >>>
> >>> Hi,
> >>>
> >>> Please review an one-line fix and a bunch of cleanups.
> >>>
> >>> The reported issue was caused by a missed setting when the
> >>> XMLStreamReader initializes the XML 1.1 scanner, so while the changeset
> >>> involved 350 lines, the fix is really just the following:
> >>>
> >>> XMLStreamReaderImpl.java:
> >>> @@ -605,11 +604,12 @@
> >>> ...
> >>> +fEntityScanner.registerListener(fScanner);
> >>>
> >>>
> >>> All other changes are cleanups, warnings. And BTW, warnings in the jaxp
> >>> repo have come down from 5230 in 2015 to 3265, a result of a bit of
> >>> cleanups here and there when we touch those classes. Still a long
> >>> way to
> >>> go, and it shows we may need to have a few dedicated patches.
> >>>
> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8167340
> >>> webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/
> >>>
> >>> Thanks,
> >>> Joe
> >

Re: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt content when size of element is > 8192

2016-12-01 Thread Joe Wang 

Thanks Christoph, Daniel! I've updated the test, removing the comments.

As for 80 chars line limit, or slightly longer than that is fine with 
me. Given our codebase, it would be an enormous undertaking. I'm 
therefore fine with taking care of only the extremely long ones or any 
that impedes the reviews, that is, side-by-side view.


Best,
Joe

On 12/1/16, 2:25 AM, Daniel Fuchs wrote:

Hi Joe,

I agree with Christoph's comments below.

+1

best regards,

-- daniel

On 01/12/16 07:40, Langer, Christoph wrote:

Hi Joe,

to me this looks good.

Did you already remove the cleanups from 
http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ ? I can't see 
a lot of them any more...


A few minor points:

It seems you still have left some debugging code in 
test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest.java:


59 while (xmlStreamReader.hasNext()) {
  60 int event = xmlStreamReader.next();
  61 if (event == XMLStreamConstants.START_ELEMENT) {
  62 if 
(xmlStreamReader.getLocalName().equals("body"))// && bMessage)


-> remove the && bMessage)

  63 {
  64 String elementText = 
xmlStreamReader.getElementText();
  65 //System.out.println("elementText=" + 
elementText + "EndOfElementText");


-> the commented System.out.println statement should be removed at 
all, I suggest


  66 // fail if elementText contains ""
  67 
Assert.assertTrue(!elementText.contains(""), "Fail: 
elementText contains ");

  68 }
  69 }
  70 }

Other than that I'm wondering if the 80 chars line limit shall be 
held which is not completely true in StreamReaderTest.java. But I 
know how difficult that is, while keeping the code still readable. 
Especially with the long speaking Class and method names ;-)


Best regards
Christoph



-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] 
On Behalf

Of Joe Wang
Sent: Mittwoch, 30. November 2016 22:21
To: core-libs-dev@openjdk.java.net
Subject: RFR (JAXP) 8167340: XMLStreamReader.getElementText return 
corrupt

content when size of element is > 8192

Hi,

Please review an one-line fix and a bunch of cleanups.

The reported issue was caused by a missed setting when the
XMLStreamReader initializes the XML 1.1 scanner, so while the changeset
involved 350 lines, the fix is really just the following:

XMLStreamReaderImpl.java:
@@ -605,11 +604,12 @@
...
+fEntityScanner.registerListener(fScanner);


All other changes are cleanups, warnings. And BTW, warnings in the jaxp
repo have come down from 5230 in 2015 to 3265, a result of a bit of
cleanups here and there when we touch those classes. Still a long 
way to

go, and it shows we may need to have a few dedicated patches.

JBS: https://bugs.openjdk.java.net/browse/JDK-8167340
webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/

Thanks,
Joe

Re: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt content when size of element is > 8192

2016-12-01 Thread Joe Wang 

Thanks Lance!

I've re-created the webrev with the one-line change:
webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ 



I'll use https://bugs.openjdk.java.net/browse/JDK-8170556 for the 
cleanup, will expand to cover the stream package, but leave the impl 
package for a later time.


Thanks,
Joe


On 11/30/16, 5:07 PM, Lance Andersen wrote:

Hi Joe

Looks OK.

I might suggest a separate issue for the cleanup and just have this 
bug address the  bug fix


Best
Lance
On Nov 30, 2016, at 4:21 PM, Joe Wang > wrote:


Hi,

Please review an one-line fix and a bunch of cleanups.

The reported issue was caused by a missed setting when the 
XMLStreamReader initializes the XML 1.1 scanner, so while the 
changeset involved 350 lines, the fix is really just the following:


XMLStreamReaderImpl.java:
@@ -605,11 +604,12 @@
...
+fEntityScanner.registerListener(fScanner);


All other changes are cleanups, warnings. And BTW, warnings in the 
jaxp repo have come down from 5230 in 2015 to 3265, a result of a bit 
of cleanups here and there when we touch those classes. Still a long 
way to go, and it shows we may need to have a few dedicated patches.


JBS: https://bugs.openjdk.java.net/browse/JDK-8167340
webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ 



Thanks,
Joe




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt content when size of element is > 8192

2016-12-01 Thread Daniel Fuchs 

Hi Joe,

I agree with Christoph's comments below.

+1

best regards,

-- daniel

On 01/12/16 07:40, Langer, Christoph wrote:

Hi Joe,

to me this looks good.

Did you already remove the cleanups from 
http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ ? I can't see a lot of 
them any more...

A few minor points:

It seems you still have left some debugging code in 
test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest.java:

59 while (xmlStreamReader.hasNext()) {
  60 int event = xmlStreamReader.next();
  61 if (event == XMLStreamConstants.START_ELEMENT) {
  62 if (xmlStreamReader.getLocalName().equals("body"))// && 
bMessage)

-> remove the && bMessage)

  63 {
  64 String elementText = xmlStreamReader.getElementText();
  65 //System.out.println("elementText=" + elementText + 
"EndOfElementText");

-> the commented System.out.println statement should be removed at all, I 
suggest

  66 // fail if elementText contains ""
  67 Assert.assertTrue(!elementText.contains(""), "Fail: 
elementText contains ");
  68 }
  69 }
  70 }

Other than that I'm wondering if the 80 chars line limit shall be held which is 
not completely true in StreamReaderTest.java. But I know how difficult that is, 
while keeping the code still readable. Especially with the long speaking Class 
and method names ;-)

Best regards
Christoph



-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf
Of Joe Wang
Sent: Mittwoch, 30. November 2016 22:21
To: core-libs-dev@openjdk.java.net
Subject: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt
content when size of element is > 8192

Hi,

Please review an one-line fix and a bunch of cleanups.

The reported issue was caused by a missed setting when the
XMLStreamReader initializes the XML 1.1 scanner, so while the changeset
involved 350 lines, the fix is really just the following:

XMLStreamReaderImpl.java:
@@ -605,11 +604,12 @@
...
+fEntityScanner.registerListener(fScanner);


All other changes are cleanups, warnings. And BTW, warnings in the jaxp
repo have come down from 5230 in 2015 to 3265, a result of a bit of
cleanups here and there when we touch those classes. Still a long way to
go, and it shows we may need to have a few dedicated patches.

JBS: https://bugs.openjdk.java.net/browse/JDK-8167340
webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/

Thanks,
Joe

RE: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt content when size of element is > 8192

2016-11-30 Thread Langer, Christoph 
Hi Joe,

to me this looks good.

Did you already remove the cleanups from 
http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ ? I can't see a lot of 
them any more...

A few minor points:

It seems you still have left some debugging code in 
test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest.java:

59 while (xmlStreamReader.hasNext()) {
  60 int event = xmlStreamReader.next();
  61 if (event == XMLStreamConstants.START_ELEMENT) {
  62 if (xmlStreamReader.getLocalName().equals("body"))// && 
bMessage)

-> remove the && bMessage)

  63 {
  64 String elementText = xmlStreamReader.getElementText();
  65 //System.out.println("elementText=" + elementText + 
"EndOfElementText");

-> the commented System.out.println statement should be removed at all, I 
suggest

  66 // fail if elementText contains ""
  67 Assert.assertTrue(!elementText.contains(""), 
"Fail: elementText contains ");
  68 }
  69 }
  70 }

Other than that I'm wondering if the 80 chars line limit shall be held which is 
not completely true in StreamReaderTest.java. But I know how difficult that is, 
while keeping the code still readable. Especially with the long speaking Class 
and method names ;-)

Best regards
Christoph


> -Original Message-
> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf
> Of Joe Wang
> Sent: Mittwoch, 30. November 2016 22:21
> To: core-libs-dev@openjdk.java.net
> Subject: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt
> content when size of element is > 8192
> 
> Hi,
> 
> Please review an one-line fix and a bunch of cleanups.
> 
> The reported issue was caused by a missed setting when the
> XMLStreamReader initializes the XML 1.1 scanner, so while the changeset
> involved 350 lines, the fix is really just the following:
> 
> XMLStreamReaderImpl.java:
> @@ -605,11 +604,12 @@
> ...
> +fEntityScanner.registerListener(fScanner);
> 
> 
> All other changes are cleanups, warnings. And BTW, warnings in the jaxp
> repo have come down from 5230 in 2015 to 3265, a result of a bit of
> cleanups here and there when we touch those classes. Still a long way to
> go, and it shows we may need to have a few dedicated patches.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8167340
> webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/
> 
> Thanks,
> Joe

Re: RFR (JAXP) 8167340: XMLStreamReader.getElementText return corrupt content when size of element is > 8192

2016-11-30 Thread Lance Andersen 
Hi Joe

Looks OK.

I might suggest a separate issue for the cleanup and just have this bug address 
the  bug fix

Best
Lance
> On Nov 30, 2016, at 4:21 PM, Joe Wang  wrote:
> 
> Hi,
> 
> Please review an one-line fix and a bunch of cleanups.
> 
> The reported issue was caused by a missed setting when the XMLStreamReader 
> initializes the XML 1.1 scanner, so while the changeset involved 350 lines, 
> the fix is really just the following:
> 
> XMLStreamReaderImpl.java:
> @@ -605,11 +604,12 @@
> ...
> +fEntityScanner.registerListener(fScanner);
> 
> 
> All other changes are cleanups, warnings. And BTW, warnings in the jaxp repo 
> have come down from 5230 in 2015 to 3265, a result of a bit of cleanups here 
> and there when we touch those classes. Still a long way to go, and it shows 
> we may need to have a few dedicated patches.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8167340
> webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/
> 
> Thanks,
> Joe

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com