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

[Dave Bristor]

Mandy Chung wrote:
> 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.

Why would expressing XOR with other than the language-provided operator be an 
improvement?  Is this what you mean?

         if ((platform == null && arch != null)
             || (platform != null && arch == null) {

The JLS seemingly makes is clear that ^ is acceptable with booleans:

<quote>
15.22.2 Boolean Logical Operators &, ^, and |

When both operands of a &, ^, or | operator are of type boolean or Boolean, 
then the type of the bitwise operator is boolean.
</quote>

>>>     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?

It increases coupling unnecessarily.  I can't find the reference I've got in 
mind, but Google quickly turns up this:
http://www.eli.sdsu.edu/courses/spring01/cs635/notes/module/module.html#Heading2

By allowing null, the caller does not have to reference (e.g. import) 
VersionConstraint, and is therefore not coupled to it.  The caller need not 
depend on VersionConstraint unless there is some other requirement.

Also, there is precedent for using null, in that other parts of the module 
system allow one to not specify a version/constraint, implying 
VersionConstraint.DEFAULT.

>>>     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)?

Ah, right; the Repository parameter is only acting to disambiguate two 
methods, based on signatures.  I've changed the naming of the public methods 
from "visit" to "traverse".  The remaining "visit" methods are now part of an 
abstract static inner class; JRepo extends this class.  I think this is 
clearer; it separates the notion of traversal over a graph from visiting a 
particular node in the graph and that separation is by names of methods and by 
classes.

>>>   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.

Thanks for clarifiying.  I added boolean traversedAny().

>>>   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?

I like the idea of stopping at the bootstrap repository instead of based on 
name.  But, what impact will that have on standard extensions?  Certainly we 
want to show them, and their dependencies (if any).  My recollection is that 
we've not finalized how they'll be handled.

For now, I'm adopting your suggestion of using startsWith.  The change is in 
both pre- and post-Visit; I missed changing one before.

I've just uploaded the webrev to
     http://webrev.invokedynamic.info/bristor/6605077-02
but they take a few minutes to materialize.

Thanks,
	Dave

> 
> Mandy