[icedtea-web] RFC: Handle a requested heap size of '1g'

[Omair Majid]

* Jiri Vanek <jvanek at redhat.com> [2014-03-31 09:33]:
> On 03/28/2014 10:44 PM, Omair Majid wrote:
> >I just ran across this jnlp file: http://atlaslivecasino.net/casino_game.jnlp
> >
> >This fails to work with HEAD because the application asks for a heap
> >size of '1G'. This is not a regression.
> 
> What about t and T ? O:)

The man page for java suggests the valid options are: k/K/m/M/g/G.

> I do not wont to be nitpicker, but I think this code desreves a bit more refactoring>
> 
> Eg the one attached?

Sure, this looks even better!

> Feel free to push anyone you like more.

I don't feel appropriate taking credit for the much nicer patch you
wrote. Unless there is a strong reason to, please call yourself the
author :)

> ps: please jsut keep the  JREDesc a =  in tests. It removed NB  warning.

Ugh. Really?

> +++ b/netx/net/sourceforge/jnlp/JREDesc.java	Mon Mar 31 15:32:48 2014 +0200

> +        //0 or 0k/m/g is still valid as it was

Please remove this line/comment ^

> +        String heapSizeLower = heapSize.toLowerCase();
> +        boolean match = heapSizeLower.matches("\\s*\\d+[kmgtp]{0,1}\\s*");

'?' is the same as '{0,1}', right? Would that be cleaner to use?

If we are accepting leading or trailing spaces here, are we passing
those to the JVM too? Maybe we should trim leading/trailing spaces
before storing the sizes?

> +++ b/tests/netx/unit/net/sourceforge/jnlp/JREDescTest.java	Mon Mar 31 15:32:48 2014 +0200

> +        JREDesc a = new JREDesc(null, null, null, "1", null, null);

> +        JREDesc.checkHeapSize("10");

These two lines are doing pretty much exactly the same thing, but two
different ways. I think the tests should check the heap size in the same
way everywhere. Either through JREDesc.checkHeapSize everywhere or
through the constructor everywhere. Is there a benefit to mixing both?

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681