RFR (JDK 12/java.base) 8213325: (props) Properties.loadFromXML does not fully comply with the spec
Joe Wang
huizhe.wang at oracle.com
Tue Dec 4 21:55:22 UTC 2018
Thanks Naoto for the fast review!
Best regards,
Joe
On 12/4/18, 1:53 PM, naoto.sato at oracle.com wrote:
> Hi Joe,
>
> Those changes in the resource bundles look fine to me.
>
> Naoto
>
> On 12/4/18 1:45 PM, Joe Wang wrote:
>> Hi Naoto and all,
>>
>> Could you please take a look at the changes to the resource files for
>> tests of JDK-8172365 & JDK-8210408?
>>
>> Current webrevs:
>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v04/
>>
>> Previous:
>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/
>>
>> Thanks,
>> Joe
>>
>> On 11/16/18, 2:27 PM, Joe Wang wrote:
>>> Hi Daniel,
>>>
>>> On 11/16/18, 10:32 AM, Daniel Fuchs wrote:
>>>> Hi Joe,
>>>>
>>>> 1. It looks as if the parser will not complain if the root element
>>>> is seen twice - and it looks as if it is a supported feature
>>>> since the previous implementation also had a counter.
>>>>
>>>> Should this be tested?
>>>> If this is a feature and multiple roots are supported
>>>> then the new impl might have an issue with allowing only
>>>> one comment (maybe).
>>>
>>> Removed "properties" from the ALLOWED_ELEMENTS so that it will
>>> report an error on properties inside properties, and added a test
>>> (IllegalElement.xml).
>>>>
>>>> 2. The rootElm qName is now set at two locations. Is the overridden
>>>> method below still needed? Could it have been used instead of
>>>> introducing a new startDTD method?
>>>>
>>>> 212 @Override
>>>> 213 public void notationDecl(String name, String publicId,
>>>> String systemId) throws SAXException {
>>>> 214 rootElm = name;
>>>> 215 }
>>>
>>> Removed the method override. startDTD is the correct place to
>>> accomplish this.
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/
>>>
>>> Previous:
>>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/
>>>
>>> Best regards,
>>> Joe
>>>
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>> On 15/11/2018 20:26, Joe Wang wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> I deleted the endDTD method. It could have been used to signal the
>>>>> end of DTD parsing and therefore serve as a validation point.
>>>>> However, the parser would have thrown Exceptions if a DTD parsing
>>>>> wasn't completed properly, that seems to make an endDTD check
>>>>> unnecessary, at least as the current parser impl goes.
>>>>>
>>>>> The small footprint parser has its problems, for example, not
>>>>> providing specific error messages when things go wrong. But since
>>>>> this is a Properties usage only, it serves the purpose well enough.
>>>>>
>>>>> Updated webrev:
>>>>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/
>>>>>
>>>>> Previous:
>>>>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v01/
>>>>>
>>>>> Best regards,
>>>>> Joe
>>>>>
>>>>> On 11/14/18, 2:32 AM, Daniel Fuchs wrote:
>>>>>> Hi Joe,
>>>>>>
>>>>>> I do not see where the new DTDHandler::endDTD methos is called.
>>>>>> Is that an oversight or is there more magic?
>>>>>>
>>>>>> best regards,
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>> On 14/11/2018 05:18, Joe Wang wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review a patch to bring the small footprint parser for
>>>>>>> java.util.Properties compliant with the specification.
>>>>>>>
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8213325
>>>>>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev/
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Joe
>>>>>>
>>>>
More information about the core-libs-dev
mailing list