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

[Rémi Forax]

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