Hi Brent,
I think all you need for test is this:
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.Stream;
public class CheckOverrides {
public static void main(String[] args) {
Set<MethodSignature> pMethodSignatures =
Stream.of(Properties.class.getDeclaredMethods())
.filter(CheckOverrides::isMethodOfInterest)
.map(MethodSignature::new)
.collect(Collectors.toSet());
Map<MethodSignature, Method> unoverriddenMethods = new HashMap<>();
for (Class<?> superclass = Properties.class.getSuperclass();
superclass != Object.class;
superclass = superclass.getSuperclass()) {
Stream.of(superclass.getDeclaredMethods())
.filter(CheckOverrides::isMethodOfInterest)
.forEach(m -> unoverriddenMethods.putIfAbsent(new
MethodSignature(m), m));
}
unoverriddenMethods.keySet().removeAll(pMethodSignatures);
if (!unoverriddenMethods.isEmpty()) {
throw new RuntimeException(
"The following methods should be overridden by
Properties class:\n" +
unoverriddenMethods.values().stream()
.map(Method::toString)
.collect(Collectors.joining("\n ", " ", "\n"))
);
}
}
static boolean isMethodOfInterest(Method method) {
int mods = method.getModifiers();
return !Modifier.isStatic(mods) &&
(Modifier.isPublic(mods) || Modifier.isProtected(mods));
}
static class MethodSignature {
final Class<?> returnType;
final String name;
final Class<?>[] parameterTypes;
MethodSignature(Method method) {
this(method.getReturnType(), method.getName(),
method.getParameterTypes());
}
private MethodSignature(Class<?> returnType, String name,
Class<?>[] parameterTypes) {
this.returnType = returnType;
this.name = name;
this.parameterTypes = parameterTypes;
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
MethodSignature that = (MethodSignature) o;
if (!returnType.equals(that.returnType)) return false;
if (!name.equals(that.name)) return false;
return Arrays.equals(parameterTypes, that.parameterTypes);
}
@Override
public int hashCode() {
int result = returnType.hashCode();
result = 31 * result + name.hashCode();
result = 31 * result + Arrays.hashCode(parameterTypes);
return result;
}
}
}
Regards, Peter
On 05/04/2015 08:32 PM, Peter Levart wrote:
On 05/04/2015 06:11 PM, Brent Christian wrote:
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I
would like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
There is some discussion of it in the bug report, starting at
2014-12-31.
The problem, as stated by Mandy:
"System Properties is a hashtable that synchronizes on itself for any
access. Currently System.getProperties returns the Properties
instance accessed by the system in which any application code might
synchronize on it (that's what the test is doing). The problem
reported JDK-6977738 is about Properties.store method that was fixed
not to synchronize on this instance. System property is a common way
for changing the default setting and so it's impractical to expect
the class loading code path not to call System.getProperty."
This fix changes java.util.Properties to store its values in an
internal ConcurrentHashMap, ignoring its Hashtable heritage. In this
way, Properties can be "de-sychronized": all methods inherited from
Hashtable are overridden, to remove synchronization, and delegate to
the internal CHM.
The serialized form is unchanged.
An alternative approach considered would be for
System.getProperties() to return a duplicate snapshot of the current
Properties. This presents a compatibility risk to existing code that
keeps a reference to the return value of System.getProperties() and
expects to either read new properties added afterwards, or set
properties on the cached copy.
-Brent
Hi Brent,
The test looks mostly good, but I would refrain from matching the
exception types as part of method signature. Why?
- javac already performs all the necessary checks about declared
exceptions if some method overrides another method
- overriding method can declare a subset and/or subtypes of checked
exceptions of those than overridden method declares and this is OK.
- overriding/overridden methods can declare arbitrary unrelated sets
of unchecked exceptions and this is OK.
Another question is whether return type should be considered part of
the method signature. For JVM it is, but Java, the language, just
matches the method's name and parameter types and javac makes sure
that overriding method declarest the same return type or a subtype of
overridden method's return type. Well, javac generates two methods in
the later case - the one declared in the source code with covariant
return type and a synthetic bridge method with overridden method's
return type that just calls the declared method.
Class.getDeclaredMethods() returns both methods in that case. So
either "key" shape would work (one that takes returnType as part of
key and one that doesn't).
I don't think you need or even want to check that Properties declares
all the methods with signatures from all interfaces. Just checking
superclass(es) would do, as Properties is not abstract and in order to
compile it must implement all abstract methods (either by inheriting
from superclass(es) or implementing them itself). Specifically, you
would not want to enforce interface default methods to be overridden
by Properties if they are not overridden by superclass(es) (as the
interface default method can only call other methods in the interface
or Object methods and you need not override such method in Properties
for Properties to be OK).
Regards, Peter
