Demo refresh

[Nils Loodin]

Hey Guys! Thanks for looking at this!

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 
> <http://cr.openjdk.java.net>, e.g.
>    scp -r 7029383/webrev.01/ nloodin at cr.openjdk.java.net 
> <mailto:nloodin at cr.openjdk.java.net>:
> Then anyone can view it with:
> http://cr.openjdk.java.net/~nloodin/webrev.01/ 
> <http://cr.openjdk.java.net/%7Enloodin/webrev.01/>
>
> My comments:
Yeah, I know I should've done that, it turns out I have some issues with 
my public/private keys. Honestly I think I might have done some error in 
copy / pasting when I signed up for the openjdk username, because I 
can't connect with scp with my private key.. I've generated a new key 
pair and mailed keys at openjdk.java.net to try to rectify it, but so far I 
haven't received any answers.. if you guys have any tips for me on how 
to get through that faster, I'm all ears.. As soon as I get the key 
worked out I'll put the webrev there of course.


>
> 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.
Yeah, my mistake. Actually Mandy Chung already pointed this out in a 
code review, and then I forgot to change that when before submitting the 
next webrev..

> 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.
In this instance, it was actually on purpose, following the previous 
example of commenting out something that wasn't relevant before. Guess 
I'll remove both then.

> 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.
Yeah, this file seems to mix several code formatting standards.. 
Sometimes two spaces are used, sometimes three and sometimes four it 
seems. Also mixed in the same function..
Also I goofed when indenting stuff I added, so that didn't help, sigh.. 
guess I'll have to use some sort of mixed spacing plugin in eclipse? ;)

I'll just reformat the whole file instead. I'll use spacing 3... there's 
some other interesting stuff going on, like the
if (true)
<return>

throw Exception() <- dead code

stuff at some places too. Not sure what I should do with that..

> 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.
Eh.. sorry about that. Copy-Paste error. I think I meant to copy the 
@Override annotation and the note that it wasn't used in the demo
and another line got copied as well from the one place where it should 
be. Embarrassing.

>
> 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 actually do as well, but I got the comments I should avoid this (i did 
it in the first rev) to minimize changes. Myself, I use ctrl-shift-O in 
eclipse compulsively :)

I guess since there's contradictory advice about this that we should 
reach some sort of agreement before proceeding?

So in conclusion, there's these things that we need to settle.

1. Keep jpda demo or remove it completely?
2. Expand imports or keep it with wildcards

I'll publish a webrev in cr.openjdk.net as soon as the key issue is 
sorted, and in the mean time you can look at the changes after applying 
your comments here:
http://wikifiles.se.oracle.com/qa/nloodin/7029383/webrev.02/ 
<http://wikifiles.se.oracle.com/qa/nloodin/7029383/webrev.00/>

Thank you all for your time and please forgive me for my beginner 
mistakes! :)

Regards
Nisse
>
> 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/  <http://wikifiles.se.oracle.com/qa/nloodin/7029383/webrev.00/>
>>
>> Regards
>> Nils Loodin
>>
>

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