Hello, Anthony.
How about this:
> line = line.trim();
> if (line.startsWith("#") || line.isEmpty()) continue;
> while (line.endsWith("\\")) {
> line = line.substring(0, line.length() - 1) + reader.readLine().trim();
> }
> int delimiterPosition = line.indexOf('=');
> String key = line.substring(0, delimiterPosition).replace("\\ ", " ");
> String[] values = line.substring(delimiterPosition + 1,
> line.length()).split(",");
With best regards. Petr.
On 15 июля 2014 г., at 14:31, Anthony Petrov <[email protected]> wrote:
> Hi Petr,
>
> I'm still a bit concerned about this line:
>
>> 243 String[] values = line.substring(delimiterPosition + 1,
>> line.length())
>> 244 .replace(",\\", ",")
>> 245 .split(",");
>
> This code now introduces another assumption, that every continuation slash is
> preceded by a comma. Also it assumes that no such a sequence of characters
> (",\") may occur anywhere else in the parsed lines.
>
> I'm not okay with these assumption. I'll reiterate my suggestion once again:
> would it make sense to only remove the actual continuation slashes?
>
> --
> best regards,
> Anthony
>
> On 7/15/2014 12:19 PM, Petr Pchelko wrote:
>> Hello, Anthony.
>>
>> Thank you for the review.
>> The new version is here:
>> http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.03/
>> Only SystemFlavorMap is changed again.
>>
>> I've fixed the parser according to your comments.
>>
>>> Also, how about processing commas within string literals? There's not a
>>> case yet, but I can imagine such a mime type definition (e.g.
>>> ...;terminators=",\\\r\n";...) (for the fun of it, i've also added the
>>> slash character in there.)
>> I don't think we should worry about it now. The properties file is internal
>> and it's highly unlikely we would ever make such a mime-type. And we can
>> update the parser when we need.
>> If I handle this situation now this would be just dead code and unneeded
>> complexity.
>>
>> With best regards. Petr.
>>
>> On 14 июля 2014 г., at 21:00, Anthony Petrov <[email protected]>
>> wrote:
>>
>>> Hi Petr,
>>>
>>> I realize that we're parsing our internal resource, however I have a few
>>> comments regarding the reliability of the parser:
>>>
>>>> 235 if (line.startsWith("#") || line.isEmpty()) continue;
>>>
>>> Can a line start with a bunch of spaces, and then start a comment with a
>>> '#' character? Should we trim() ?
>>>
>>>
>>>> 236 while (line.endsWith("\\")) {
>>>> 237 line = line.trim() + reader.readLine().trim();
>>>> 238 }
>>>> 239 line = line.replace("\\", "");
>>>
>>> The above code assumes that '\' characters are illegal in the middle of the
>>> lines because it removes all of them. Would it make sense to only remove
>>> the actual continuation slashes? For instance, take a look at:
>>>
>>> src/windows/classes/sun/awt/datatransfer/flavormap.properties
>>>> 54 UNICODE\ TEXT=text/plain;charset=utf-16le;eoln="\r\n";terminators=2
>>>
>>> Your parser will remove the escape slashes from the "\r\n" string.
>>>
>>> However, note that we still should handle the '\ ' sequence in the key part
>>> of each line.
>>>
>>> Also, how about processing commas within string literals? There's not a
>>> case yet, but I can imagine such a mime type definition (e.g.
>>> ...;terminators=",\\\r\n";...) (for the fun of it, i've also added the
>>> slash character in there.)
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 7/14/2014 6:23 PM, Petr Pchelko wrote:
>>>> Hello, AWT Team.
>>>>
>>>> My apologies, but I've found a problem in the AWT part of this fix and so
>>>> I need one more review iteration.
>>>>
>>>> The new version is located here:
>>>> http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.02/
>>>> The only changes comparing to the previous approved version are in
>>>> SystemFlavorMap.
>>>>
>>>> I've realized that we cannot use Properties to load the
>>>> flavormap.properties file because Properties doesn't preserve the original
>>>> order of keys.
>>>> And it's important, because natives that are found in the file first are
>>>> treated as more important than natives in the end of the file.
>>>> So I've implemented a custom parser for the properties file (see lines
>>>> 233-243)
>>>>
>>>> Thank you.
>>>> With best regards. Petr.
>>>>
>>>> On 10 июля 2014 г., at 20:22, Mike Duigou <[email protected]> wrote:
>>>>
>>>>> The makefile changes look fine to me. (The use of OPENJDK as part of the
>>>>> variable names seems excessive but that's not something your patch adds
>>>>> or changes)
>>>>>
>>>>> Mike
>>>>>
>>>>> On Jul 9 2014, at 23:55 , Petr Pchelko <[email protected]> wrote:
>>>>>
>>>>>> Hello, Build Team.
>>>>>>
>>>>>> This is a reminder. Could you please review the build part of the
>>>>>> following fix:
>>>>>> http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
>>>>>>
>>>>>> Thank you.
>>>>>> With best regards. Petr.
>>>>>>
>>>>>> On 02 июля 2014 г., at 15:13, Anthony Petrov <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>>> Thanks. Note that your email editor messed up the HTML part of the
>>>>>>> email (see below a text-only quote), so here's the correct link to the
>>>>>>> webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
>>>>>>>
>>>>>>> The fix looks fine to me.
>>>>>>>
>>>>>>> --
>>>>>>> best regards,
>>>>>>> Anthony
>>>>>>>
>>>>>>> On 7/2/2014 3:10 PM, Petr Pchelko wrote:
>>>>>>>> Hello, Anthony, Alan.
>>>>>>>>
>>>>>>>> Thank you for the review, the new version is located here:
>>>>>>>> http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.01/
>>>>>>>> <http://cr.openjdk.java.net/%7Epchelko/9/8047336/webrev.00/>
>>>>>>>>
>>>>>>>> I've changed the code to throw an InternalError if we cannot read
>>>>>>>> properties.
>>>>>>>> Once I get feedback from the build team - I'll push this fix.
>>>>>>>>
>>>>>>>> With best regards. Petr.
>>>>>>>>
>>>>>>>> On 02 июля 2014 г., at 13:52, Alan Bateman <[email protected]
>>>>>>>> <mailto:[email protected]>> wrote:
>>>>>>>>
>>>>>>>>> On 01/07/2014 09:35, Petr Pchelko wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> The changes in the public API have been approved, so let me continue
>>>>>>>>>> the review process.
>>>>>>>>>>
>>>>>>>>>> For your convenience:
>>>>>>>>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8047336
>>>>>>>>>> The fix: http://cr.openjdk.java.net/~pchelko/9/8047336/webrev.00/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Epchelko/9/8047336/webrev.00/>
>>>>>>>>>>
>>>>>>>>>> Until now we've been discussing an abstract question of moving the
>>>>>>>>>> properties to a different location and agreed that it's possible.
>>>>>>>>>> Now it's time for an official code review. Could someone please make
>>>>>>>>>> one?
>>>>>>>>>>
>>>>>>>>>> Also some feedback from the build team is still required. Could
>>>>>>>>>> someone from the build team review this fix please?
>>>>>>>>>> (The question is that I've made a separate explicit rule for
>>>>>>>>>> flavormap.properties file. If I add it to COPY_PATTERNS than the
>>>>>>>>>> solaris version get's copied on Mac. Is that a bug in the build
>>>>>>>>>> system? Is my current solution good enough?)
>>>>>>>>>>
>>>>>>>>> I think this looks okay. I see Anthony's mail asking about the warning
>>>>>>>>> message and whether it should the print stack trace. I just wonder if
>>>>>>>>> it might be saner to just throw InternalError here. It throws
>>>>>>>>> InternalError if flavormap.properties is missing and it might be
>>>>>>>>> consistent to do the same when the file is corrupt or can't be parsed
>>>>>>>>> as a properties file.
>>>>>>>>>
>>>>>>>>> -Alan.
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>
