[rfc][icedtea-web] Code cleanup patches

[Andrew Azores]

On 02/04/2014 01:24 AM, Jacob Wisor wrote:
> On 02/03/2014 09:32 PM, Andrew Azores wrote:
>> Hi,
>>
>> The larger two of the three attached patches do a few simple things:
>> 1) make variables/parameters/fields final, where applicable. I feel 
>> this makes
>> it more clear which variables are used eg as accumulators or will be 
>> returned by
>> the method, etc. This also allows the removal of some local variables 
>> which were
>> created simply to have "final versions" of other variables, in order 
>> for them to
>> be captured by anonymous inner classes for example.
>> 2) prefix fields with "this" in very long methods. In shorter methods 
>> it can
>> remain clear that fields are fields since no declaration for that 
>> name can be
>> seen, but in very long methods, it can become more confusing.
>> 3) fix formatting. Eg remove useless comments, remove whitespace from 
>> "empty"
>> lines, remove trailing whitespace
>
> Looks good to me.
>
> Just on nit:
>
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java 
> b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> [...]
> -            tracker.addResource(jar.getLocation(),
> -                    jar.getVersion(), file.getDownloadOptions(),
> +           this. tracker.addResource(jar.getLocation(),
> +                    jar.getVersion(), this.file.getDownloadOptions(),
>
> Please remove the space between this. and tracker.

Ah, nice catch :)

>
> You may also want to check for "){" malformed block openings and 
> replace them with ") {".

Yea, might as well. And we should probably get stricter on this kind of 
thing during review, too.

>
> Most of the exception parameters of catch clauses do not need to be 
> explicitly marked final because they are never assigned to hence they 
> become /effectively/ /final/. The OpenJDK compiler does take this into 
> account when generating byte code indeed. But, as I understand the 
> purpose of these patches was to increase readability.
>
> Jacob

Pretty much just for readability's sake, yes.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: classloader_cleanup.patch
Type: text/x-patch
Size: 84853 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140204/a9ec4c5c/classloader_cleanup-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: classloader_cleanup-1.4.patch
Type: text/x-patch
Size: 86218 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140204/a9ec4c5c/classloader_cleanup-1.4-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extensionjnlpsinapplet_makefile_ls.patch
Type: text/x-patch
Size: 462 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140204/a9ec4c5c/extensionjnlpsinapplet_makefile_ls-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: securitydialogmessagehandler_cleanup.patch
Type: text/x-patch
Size: 8375 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140204/a9ec4c5c/securitydialogmessagehandler_cleanup-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: securitydialogmessagehandler_cleanup-1.4.patch
Type: text/x-patch
Size: 7555 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140204/a9ec4c5c/securitydialogmessagehandler_cleanup-1.4-0001.patch