[modules-dev] Review please for launcher bugs

[Dave Bristor]

Mostly fine, a few questions and minor/typo kinds of things.

	Dave

- src/share/classes/sun/module/ModuleLauncher.java

147     private static Repository repository = null;

Please put this and all other field declarations at the top of the file, not 
in the middle.

  188         String fname = new File(jamFileName).getName();
  189         String moduleName = fname.substring(0, fname.lastIndexOf('.'));
  190         List<ModuleArchiveInfo> ilist =  repository.list();
  191         for (ModuleArchiveInfo i : ilist) {
  192             if (i.getFileName().endsWith(fname + JamUtils.EXPANDED_EXT)) {
  193                 definition = getModuleDefinition(repository, i.getName());
  194                 break;
  195             }
  196         }

I remain somewhat bewildered by this: could you explain?  Also, line 188 could be
	String fname = baseDir.getName();
though perhaps baseDir should be renamed jamFile (since it's not a base 
directory).

  207             if (!jamFileName.endsWith(".jam") && 
!jamFileName.endsWith(".jar")) {
  208                 throw new IllegalArgumentException("jam filename must 
have a .jam or .jar extension");

Shouldn't .jam.pack.gz also be allowed?  Or is that just not-yet-implemented? 
  (I know it's not fully supported elsewhere...)

  231                         throw new Exception("Repository not found at " + 
repFile.toString());

Should this also be IllegalArgumentException?

- test/java/module/basic/BasicLauncherTests.java

  260     static String expRepository;
  261     static String jamRepository;
  262     static String urlRepository ;

Extra space in line 262 and please put fields at beginning of file.

Several other places, why the change from expRepository to urlRepository?  I 
might have thought an addition would be appropriate; was removal of 
expRepository intentional?

Kumar Srinivasan wrote:
> Hi,
> 
> I need a review of these fixes asap.
> http://javaweb.sfbay/~ksrini/webrevs/j2se-tools/jsr277-6/
> [The webrev contains the CRs].
> 
> Built and tested on Solaris and Windows all the regresssion
> tests in test/java/module pass.
> 
> Thanks
>