hg: jdk7/tl/langtools: 6907575: [classfile] add support for classfile dependency analysis
Jonathan Gibbons
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.
>
> 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
More information about the compiler-dev
mailing list