Demo refresh

[Kelly O'Hair]

On Apr 21, 2011, at 6:43 AM, Nils Loodin wrote:

> Key issue is now resolved, and this can be looked at (by anyone!) here:
> http://cr.openjdk.java.net/~nloodin/7029383/webrev.02/
> 
> So there's two things, as I said:
> 1. Expand imports or leave them alone

I would probably leave them alone. It was just stating my preference on 'import style'.

> 
> 2. Keep jpda demo as is / keep it with these "light changes" / remove completely

Completely removing a demo is easier said than done, in my opinion, I would leave it in.
If you or someone else started looking into the demos for jdk8, early in the release, I would
recommend removing it then.

> 
> Opinions?
> 

Keep in mind that I'm just an old retired serviceability engineer, one that keeps sticking
his nose in to serviceability places. ;^)

> I guess if there aren't any and I had my way I'd say that I'd like to
> expand imports and remove the com/sun/tools/example/debug directory (where the code for the jpda demo resides)
> 

I would not have an issue with that plan.

---
Our code review process is rather informal, mostly we want people to look for horribly obvious mistakes
and maybe minor corrections. If a reviewer thinks something is just completely broken, they will say so.
Sometimes suggestions come up, but they are often just suggestions, if they really insist on something
changing they will say so.  Controversial changes are rare, unless you purposely make it controversial. ;^)

Once again, thank you for this cleanup.

-kto

> /Nisse
> 
> On 04/18/2011 08:13 PM, Kelly O'Hair wrote:
>> 
>> Hi Nils.
>> 
>> This is fantastic, these demos needed some attention.
>> 
>> You should scp the webrev to cr.openjdk.java.net, e.g.
>>    scp -r 7029383/webrev.01/  nloodin at cr.openjdk.java.net:
>> Then anyone can view it with:
>>   http://cr.openjdk.java.net/~nloodin/webrev.01/
>> 
>> My comments:
>> 
>> src/share/classes/com/sun/tools/example/debug/bdi/ChildSession.java
>>   If the quit methods are to be deleted, we should just delete the lines, not comment out the methods.
>> src/share/classes/com/sun/tools/example/debug/bdi/SourceNameReferenceTypeSpec.java
>>   Dito on commented out code. I'd rather see it deleted, maybe leave just a plain comment that
>>   certain exceptions are just not thrown anymore.
>> src/share/classes/com/sun/tools/example/debug/expr/ExpressionParser.java
>>   What a yucky file. :^(  I'm not sure your changes have improved this file, granted it may need
>>   some more major surgery. The long lines make it very hard to review the changes side by side.
>>   You may need to look at this file again, the indentation seems messed up in places.
>> src/share/demo/nio/zipfs/Demo.java
>>   Not sure I understand the comments on the getString()/getBytes() methods.
>>   Mentions 'two paths are equal' but I don't see the connection.
>> 
>> The @Override's are fairly clear all by themselves, but the @SuppressWarnings probably deserve
>> a short comment as to why the warning is being suppressed rather than addressed in the code.
>> Even if it is a short comment like "// Serialization not used"
>> 
>> On imports, I like that you have deleted many unneeded ones. I myself prefer to completely
>> avoid wildcard imports, using an IDE to keep them accurate, but that's just my style.
>> 
>> I'm a little concerned that we might be disturbing the dust and cobwebs on these demos, I'm
>> assuming that all the demo tests will be run, and the jdb tests, but overall I'm glad they are getting
>> some attention.
>> 
>> -kto
>> 
>> On Apr 18, 2011, at 5:01 AM, Nils Loodin wrote:
>> 
>>> Hello all!
>>> My name is Nils Loodin ,and I work in the swedish SQE team, now under 
>>> Ken chen. I've been working on the refresh demos project (PRD req. no. 
>>> 537450)
>>> 
>>> I actually sent a request for a review previously, but that mail seems to have gone missing (thanks Beehive), but it's available at the mailing list archive: 
>>> (http://mail.openjdk.java.net/pipermail/serviceability-dev/2011-April/003868.html)
>>> 
>>> Anyway, that might be for the better, since I've had time to update the webrev with some more stuff after comments from the nice Mandy Chung!
>>> 
>>> One thing to note though, is that this was a pretty time bound project, but with a lot of code to go through and update. There are definitely things to do, still. perhaps this can be reviewed as an initial effort.
>>> 
>>> (Overview here: 
>>> https://sunspace.sfbay.sun.com/display/JAVASE/JDK+7+Demos+Update)
>>> 
>>> The changes should be pretty trivial. Mostly added annotations, using 
>>> for-each constructs and generics instead of raw types.
>>> 
>>> Please have a look here:
>>> http://wikifiles.se.oracle.com/qa/nloodin/7029383/webrev.01/
>>> 
>>> Regards
>>> Nils Loodin
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20110421/bbd0a358/attachment.html