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