hg: jdk7/tl/langtools: 6907575: [classfile] add support for classfile dependency analysis

[Jonathan Gibbons]

Rémi Forax wrote:
> Le 12/12/2009 18:33, jonathan.gibbons at sun.com a écrit :
>> Changeset: fbeb560f39e7
>> Author:    jjg
>> Date:      2009-12-12 09:28 -0800
>> URL:       http://hg.openjdk.java.net/jdk7/tl/langtools/rev/fbeb560f39e7
>>
>> 6907575: [classfile] add support for classfile dependency analysis
>> Reviewed-by: ksrini
>>
>> + src/share/classes/com/sun/tools/classfile/Dependencies.java
>> + src/share/classes/com/sun/tools/classfile/Dependency.java
>> + test/tools/javap/classfile/deps/GetDeps.java
>> + test/tools/javap/classfile/deps/T6907575.java
>> + test/tools/javap/classfile/deps/T6907575.out
>> + test/tools/javap/classfile/deps/p/C1.java
>>
>>    
>
> Hi Jon,
> I have some comments about this patch set.
>
> Dependency:
> - Finder.findDependencies return an Iterable of wilcard.
>   Using wildcard as return type is not a good idea because you
>   require the caller of the method to deal with wildcard.
>   Moreover, it's alway better to return a subtype than a super type
>   if they are both abstract.
>   Here: Iterable<Dependency> is better.
>
>   The doc comment of the return type is not up to date with the 
> current signature,
>   there is not set anymore. Perhaps a Collection will better fit here 
> (i.e an Iterable +
>   a size) because it will allow to use the return value as arguments 
> of methods like addAll().
>
> Dependencies:
>   - ClassFileNotFoundException, field is declared at the end of the 
> class,
>     this is not the usual place.
>   - the constructor with two parameter should call super with two 
> parameters instead
>     of using initCause() which is a public method (and synchronized ?).
>
>   - ClassFileError, same remark about initCause.
>
>   - I don't understand why the filter and the finder aren't taken as 
> argument
>     of the constructor of dependencies, it will avoid all lazy 
> initialisation stuff.
>     I think it's simpler to declare filter and finder final.
>
>   - getDefaultFinder() should be perhaps renamed to something like 
> createDefaultFinder.
>
>   - class DefaultFilter doesn't define any constructor, so a constructor
>      with default visibility is inserted. I think this constructor 
> should be private.
>      Lazy initialisation in instance doesn't seem useful since the 
> class is only loaded
>      when an instance is needed.
>
>    - in TargetRegexFilter, field "pattern" should be declared private 
> and final.
>
>    - in TargetPackageFilter, same remarks !
>
>    - in BasicDependencyFinder,
>        field locations should be private and typed as a HashMap
>        (this field is not visible).
>
>      - in Visitor,
>        deps should not be private because this field is accessed out 
> the class Visitor.
>        Currently the compiler generates an accessor for this field.
>
> Cheers,
> Rémi
>
>
>
>
Rémi,

Thanks for your comments. I'll have to go through them later with the 
code in front of me.

-- Jon