[modules-dev] Review request for 6605077, "jrepo dependencies"

[Mandy Chung]

Dave Bristor wrote:

> Mandy Chung wrote:
>
>> Hi Dave,
>>
>> Looks good in general.  Here are my review comments:
>>
>> sun/module/repository/URLModuleInfo.java
>>     line 61:  interesting to see "^" used here
>>     - this is not your change but do you mind fixing this to use "||" 
>> (OR sign) as part of your fix?
>
>
> That would not be right: ^ is XOR, not OR, and exclusive-or is 
> intended, so it's correct as-is.

My main point is that I would expand the condition check with OR and AND 
signs than using bit-wise operator in the expression of an if-statement.

>>     line 102 & 126: would it be better to have this constructor 
>> requiring a non-null VersionConstraint?  The caller should supply 
>> VersionConstraint.DEFAULT instead of null.   Also, JRepo.java is 
>> passing the default version constraint instead of null.
>
>
> I prefer allowing null; this allows less coupling from the caller to 
> modules code.  I.e., with your suggestion, the caller must directly 
> reference the VersionConstraint class.

What's the problem having the caller to reference the VersionConstraint 
class?

>
>>     line 145: repo in the visit(Repository, Module) method is not 
>> used.  Look like better to rename visit(Repository, Module) method to 
>> a different method name e.g. traverse(Module m) to begin visiting the 
>> module and its imports.
>
>
> The "repo" parameter is used in line 156.
>
"repo" is used for the recursive call but it's not really needed in the 
method implementation.  Having visit(Module m) and visit(Repository, 
Module) the same method name but different semantics is confusing.  What 
is the issue of renaming to traverse(Module m)? 

>>   line 635-637: I think it's useful to add a method in the 
>> ImportTraverser class to determine if any module has been visited 
>> (e.g. int visitedModuleCount())
>
>
> We already have
>     boolean visited(Module m)
> What's the use case for knowing the count of visits?
>
line 635-637:
    for (Module dep : iv) {
          return true;
    }

What you want above is to find out if any module as visited.  I'd like 
to see this for-loop be replaced by a simple method call.   It can just 
return true if 1 or more modules were visited and it doesn't need to 
return the count of visits but just my suggestion.

>>   line 694 & 710: should we check the name starting with "java.se" 
>> instead of equals?  We also have a "java.se.core" module, right?
>
>
> Yes, but this is intended to stop the traversal precisely at the 
> java.se module, so that nothing under it, regardless of name, is 
> traversed.  If there were some other module whose name started with 
> "java.se" that is not under "java.se", we would want to traverse it.  
> Maybe that's an illegal module name?  I'm not sure.  But currently 
> it's intentional as-is.
>
A module can import "java.se.core" without importing "java.se".   So 
jrepo should stop the traversal of the imports for "java.se.core" as 
well.  It's fine with me if you want to hard code the check for both 
"java.se" and "java.se.core". 

Another suggestion is to stop the traversal for any modules in the 
bootstrap repository because they all come from the platforms.  What do 
you think?

Mandy