Re: Review Request: RT-37788
Try #2, after running it through the the Xcode leak analyzer and unit testing
(FYI keep your persistent domains lower case).
WebRev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.01/rt.patch
JIRA: https://javafx-jira.kenai.com/browse/RT-37788
And let’s move further discussion to the JIRA issue.
On Jul 2, 2014, at 10:22 PM, Petr Pchelko petr.pche...@oracle.com wrote:
Hello, Danno.
I’ve noticed that you are leaking userDefaults and expandedOptions objects.
Adding autorelease to them would save you some memory.
With best regards. Petr.
On Jul 3, 2014, at 1:43 AM, Danno Ferrin danno.fer...@oracle.com wrote:
Yes, this has to be fixed in native code. 8u40 it is then.
I can make it cause a crash, but that starts with shooting myself in the
foot, and not much can be done to fix it (by passing in bogus VM arguments).
On Jul 2, 2014, at 3:40 PM, Stephen F Northover
steve.x.northo...@oracle.com wrote:
Personally, I wouldn't change any native code at this point unless it was
fixing a crash. The review is for 8u40, correct?
Steve
On 2014-07-02, 5:38 PM, Chris Bensen wrote:
I’m not sure about for 8u20. Seems fairly straight forward, and your Obj-C
seems as good as any Obj-C. My only complaint at the moment is the
following:
358 if ([pathParts count] 2) {
359 // for 3 or more steps, the domain is first.second.third and
the keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
367 } else {
368 // for 1 or two steps, the domain is first.second.third and
the keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
371 }
what if [pathParts count] is 0? I’d probably do a switch:
switch ([pathParts count]) {
case 0:
//error
return/break;
case 1:
case 2:
368 // for 1 or two steps, the domain is first.second.third and
the keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
default:
359 // for 3 or more steps, the domain is first.second.third and
the keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
}
Make sense? Clear as mud?
Chris
On Jul 2, 2014, at 2:15 PM, Danno Ferrin danno.fer...@oracle.com wrote:
Chris, Kevin, Steve,
Please review this fix for RT-37788. Since I am not an objective C any
comments are welcome. Also, please consider if this is too much for an
8u20 fix (the diff is against the current 8u40 codebase).
Webrev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.00/
JIRA: https://javafx-jira.kenai.com/browse/RT-37788
—Danno
Review Request: RT-37788
Chris, Kevin, Steve, Please review this fix for RT-37788. Since I am not an objective C any comments are welcome. Also, please consider if this is too much for an 8u20 fix (the diff is against the current 8u40 codebase). Webrev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.00/ JIRA: https://javafx-jira.kenai.com/browse/RT-37788 —Danno
Re: Review Request: RT-37788
I'm glad your not an objective C. I suggest getting Felipe to look at this code too should we decide to go ahead with it. Steve On 2014-07-02, 5:15 PM, Danno Ferrin wrote: Chris, Kevin, Steve, Please review this fix for RT-37788. Since I am not an objective C any comments are welcome. Also, please consider if this is too much for an 8u20 fix (the diff is against the current 8u40 codebase). Webrev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.00/ JIRA: https://javafx-jira.kenai.com/browse/RT-37788 —Danno
Re: Review Request: RT-37788
I’m not sure about for 8u20. Seems fairly straight forward, and your Obj-C
seems as good as any Obj-C. My only complaint at the moment is the following:
358 if ([pathParts count] 2) {
359 // for 3 or more steps, the domain is first.second.third and the
keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
367 } else {
368 // for 1 or two steps, the domain is first.second.third and the
keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
371 }
what if [pathParts count] is 0? I’d probably do a switch:
switch ([pathParts count]) {
case 0:
//error
return/break;
case 1:
case 2:
368 // for 1 or two steps, the domain is first.second.third and the
keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
default:
359 // for 3 or more steps, the domain is first.second.third and the
keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
}
Make sense? Clear as mud?
Chris
On Jul 2, 2014, at 2:15 PM, Danno Ferrin danno.fer...@oracle.com wrote:
Chris, Kevin, Steve,
Please review this fix for RT-37788. Since I am not an objective C any
comments are welcome. Also, please consider if this is too much for an 8u20
fix (the diff is against the current 8u40 codebase).
Webrev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.00/
JIRA: https://javafx-jira.kenai.com/browse/RT-37788
—Danno
Re: Review Request: RT-37788
Personally, I wouldn't change any native code at this point unless it
was fixing a crash. The review is for 8u40, correct?
Steve
On 2014-07-02, 5:38 PM, Chris Bensen wrote:
I’m not sure about for 8u20. Seems fairly straight forward, and your
Obj-C seems as good as any Obj-C. My only complaint at the moment is
the following:
358 if ([pathParts count] 2) {
359 // for 3 or more steps, the domain is first.second.third and the keys are
/first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
367 } else {
368 // for 1 or two steps, the domain is first.second.third and the keys are /,
first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
371 }
what if [pathParts count] is 0? I’d probably do a switch:
switch ([pathParts count]) {
case 0:
//error
return/break;
case 1:
case 2:
368 // for 1 or two steps, the domain is first.second.third and the keys are /,
first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
default:
359 // for 3 or more steps, the domain is first.second.third and the keys are
/first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
}
Make sense? Clear as mud?
Chris
On Jul 2, 2014, at 2:15 PM, Danno Ferrin danno.fer...@oracle.com
mailto:danno.fer...@oracle.com wrote:
Chris, Kevin, Steve,
Please review this fix for RT-37788. Since I am not an objective C
any comments are welcome. Also, please consider if this is too much
for an 8u20 fix (the diff is against the current 8u40 codebase).
Webrev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.00/
http://cr.openjdk.java.net/%7Eshemnon/RT-37788/webrev.00/
JIRA: https://javafx-jira.kenai.com/browse/RT-37788
—Danno
Re: Review Request: RT-37788
Yes, this has to be fixed in native code. 8u40 it is then.
I can make it cause a crash, but that starts with shooting myself in the foot,
and not much can be done to fix it (by passing in bogus VM arguments).
On Jul 2, 2014, at 3:40 PM, Stephen F Northover steve.x.northo...@oracle.com
wrote:
Personally, I wouldn't change any native code at this point unless it was
fixing a crash. The review is for 8u40, correct?
Steve
On 2014-07-02, 5:38 PM, Chris Bensen wrote:
I’m not sure about for 8u20. Seems fairly straight forward, and your Obj-C
seems as good as any Obj-C. My only complaint at the moment is the following:
358 if ([pathParts count] 2) {
359 // for 3 or more steps, the domain is first.second.third and
the keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
367 } else {
368 // for 1 or two steps, the domain is first.second.third and the
keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
371 }
what if [pathParts count] is 0? I’d probably do a switch:
switch ([pathParts count]) {
case 0:
//error
return/break;
case 1:
case 2:
368 // for 1 or two steps, the domain is first.second.third and the
keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
default:
359 // for 3 or more steps, the domain is first.second.third and
the keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
}
Make sense? Clear as mud?
Chris
On Jul 2, 2014, at 2:15 PM, Danno Ferrin danno.fer...@oracle.com wrote:
Chris, Kevin, Steve,
Please review this fix for RT-37788. Since I am not an objective C any
comments are welcome. Also, please consider if this is too much for an
8u20 fix (the diff is against the current 8u40 codebase).
Webrev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.00/
JIRA: https://javafx-jira.kenai.com/browse/RT-37788
—Danno
Re: Review Request: RT-37788
Hello, Danno.
I’ve noticed that you are leaking userDefaults and expandedOptions objects.
Adding autorelease to them would save you some memory.
With best regards. Petr.
On Jul 3, 2014, at 1:43 AM, Danno Ferrin danno.fer...@oracle.com wrote:
Yes, this has to be fixed in native code. 8u40 it is then.
I can make it cause a crash, but that starts with shooting myself in the
foot, and not much can be done to fix it (by passing in bogus VM arguments).
On Jul 2, 2014, at 3:40 PM, Stephen F Northover
steve.x.northo...@oracle.com wrote:
Personally, I wouldn't change any native code at this point unless it was
fixing a crash. The review is for 8u40, correct?
Steve
On 2014-07-02, 5:38 PM, Chris Bensen wrote:
I’m not sure about for 8u20. Seems fairly straight forward, and your Obj-C
seems as good as any Obj-C. My only complaint at the moment is the
following:
358 if ([pathParts count] 2) {
359 // for 3 or more steps, the domain is first.second.third and
the keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
367 } else {
368 // for 1 or two steps, the domain is first.second.third and the
keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
371 }
what if [pathParts count] is 0? I’d probably do a switch:
switch ([pathParts count]) {
case 0:
//error
return/break;
case 1:
case 2:
368 // for 1 or two steps, the domain is first.second.third and the
keys are /, first/, second/
369 persistentDomain = @DEFAULT_JAVA_PREFS_DOMAIN;
370 [dictPath insertObject: @ atIndex:0];
default:
359 // for 3 or more steps, the domain is first.second.third and
the keys are /first/second/third/, fourth/, fifth/... etc
360 persistentDomain = [NSString stringWithFormat: @%@.%@.%@,
[pathParts objectAtIndex: 0],
361 [pathParts objectAtIndex: 1], [pathParts
objectAtIndex: 2]];
362
363 [dictPath replaceObjectAtIndex: 0 withObject: [NSString
stringWithFormat:@/%@/%@/%@, [pathParts objectAtIndex: 0],
364[pathParts
objectAtIndex: 1], [pathParts objectAtIndex: 2]]];
365 [dictPath removeObjectAtIndex: 2];
366 [dictPath removeObjectAtIndex: 1];
}
Make sense? Clear as mud?
Chris
On Jul 2, 2014, at 2:15 PM, Danno Ferrin danno.fer...@oracle.com wrote:
Chris, Kevin, Steve,
Please review this fix for RT-37788. Since I am not an objective C any
comments are welcome. Also, please consider if this is too much for an
8u20 fix (the diff is against the current 8u40 codebase).
Webrev: http://cr.openjdk.java.net/~shemnon/RT-37788/webrev.00/
JIRA: https://javafx-jira.kenai.com/browse/RT-37788
—Danno
