BuiltInClass as coded is very risky. It has three separate static
initializer blocks. It also has dozens of static fields, each
initialized by invoking a static method 'addClass(Symbol)' which in
turn invokes its parent class static method 'LispClass.addClass
(Symbol, LispClass)'.
I attach a simplified failing JUnit testcase which exposes how we
cannot rely on the JVM always executing these separate bits of code
in the order we expect.
What happens is this: the parent class, LispClass, has a static final
field, 'map', which on a naive view seems deterministically certain
to be initialized before it is needed. We need it when we invoke the
BuiltInClass 'addClass' method, which in turn invokes the LispClass
'addClass' method, wherein the 'map' field is dereferenced. But when
you run the testcase in the debugger, you will see that 'map' is null
-- which looks like it should be impossible, but in fact is not; the
hitherto "reliable" initialization of the 'map' field is really an
accidental by-product of the most common use case.
I propose to fix this by moving most of the static initializer block
processing into overloaded private BuiltInClass constructors. Each
constructor will call LispClass.addClass (directly or indirectly),
thus retaining the intended functionality. So it's a refactoring:
"introduce constructor".
This will have the side effect of removing a lot of duplicate
boilerplate code within the large static initializer block.
|