hg: jdk7/tl/langtools: 6907575: [classfile] add support for classfile dependency analysis
Rémi Forax
forax at univ-mlv.fr
Sat Dec 12 15:35:01 PST 2009
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
More information about the compiler-dev
mailing list