RFR: 8237878: Improve ModuleLoaderMap datastructures

[forax at univ-mlv.fr]

----- Mail original -----
> De: "Claes Redestad" <claes.redestad at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Jeudi 6 Février 2020 14:48:38
> Objet: Re: RFR: 8237878: Improve ModuleLoaderMap datastructures

> Hi Rémi,
> 
> 
> On 2020-02-06 14:08, Remi Forax wrote:
>> Hi Claes,
>> In ArchivedModuleGraph, there is no point to take the mainModule as parameter
>> given that it should always be null.
> 
> right, I've been thinking this should be simplified for now. Once we
> implement support for archiving non-default scenarios the internal
> datastructure will need a refactoring anyhow, so let's cut things down
> to size.
> 
>> 
>> In ModuleLoaderMap, the constants PLATFORM_LOADER_INDEX and APP_LOADER_INDEX
>> should be moved inside the nested class Mapper given that there are only needed
>> by the Mapper
>> and mappingFunction should be moved inside the class Mapper too.
> 
> Done.
> 
>> 
>> in mappingFunction, i think that if you are using 'var', you should using
>> everywhere in the method,
>> to avoid to have a weird mix between local variables declared with and without
>> var.
>> so either map is not declared with var or resolvedModule and nm are declared
>> with var.
> 
> I disagree var usage should be an all or nothing ordeal, rather it
> should be used when the type information is readily available, e.g.,
> by being stated in detail on the RHS.
> 
> The exact type in the other cases would be somewhat muddied: maybe
> name() is "obviously" a String, but what type the elements in
> cf.modules() are is not at all obvious, and making code less obvious
> should be avoided.

If your variable have a name like resolvedModule then the type ResolvedModule doesn't give you more info.
Obviously, it doesn't work if the variable name is an abbreviation, but instead of not using var, i think it's better to change the variable name.

> 
> New webrev:
> 
> http://cr.openjdk.java.net/~redestad/8237878/open.01/
> 
> Re-running some tests..
> 
> /Claes

Rémi

> 
>> 
>> I believe that at some point in the future, let say just before the release of
>> 17, we should do a global pass and rewrite each class to use 'var', the same
>> way we have done with generics.
>> It will be nasty for backporting bugs but i think it's better than having a mix
>> between codes that use var and codes that doesn't use it.
>> 
>> Rémi
>> 
>> ----- Mail original -----
>>> De: "Claes Redestad" <claes.redestad at oracle.com>
>>> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>>> Envoyé: Jeudi 6 Février 2020 13:37:59
>>> Objet: RFR: 8237878: Improve ModuleLoaderMap datastructures
>> 
>>> Hi,
>>>
>>> refactor ModuleLoaderMap to allow the datastructure to be
>>> archived, then archive it.
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8237878/open.00/
>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8237878
>>>
>>> Testing: tier1-3
>>>
> >> /Claes