hg: jdk7/tl/langtools: 6907575: [classfile] add support for classfile dependency analysis
Jonathan.Gibbons at Sun.COM
Mon Dec 14 09:09:45 PST 2009
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.
> - 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().
> - ClassFileNotFoundException, field is declared at the end of the
> 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
> 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
> - 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.
Thanks for your comments. I'll have to go through them later with the
code in front of me.
More information about the compiler-dev