Remove the unused env_init field from class blocks

[lthls]

I eventually noticed (after spending some amount of time looking through the class and object code, for other reasons) that one of the four fields of class blocks was written to but never read from.
I looked around in all the suspicious places, and when I was convinced that there were indeed no accesses to this field directly or indirectly in all the places I knew where related to classes, I tentatively removed it (to find out if there were places manipulating classes that I was not aware of). The testsuite runs cleanly.

So I'm now convinced that the field isn't actually used, but I'm not sure whether it's a good idea to remove it or not. I'm submitting this PR to share my findings (and because I had written the code anyway), but I wouldn't be particularly disappointed if it was closed for one reason or another (if it's because the field is not actually unused, I want to hear about it though).

Cc @garrigue, who I assume is the person most likely to know about this.

[shindere]

Out of curiosity: did `git blame` help in any way to understand how we endedup in this situation?

[lthls]

Out of curiosity: did git blame help in any way to understand how we endedup in this situation?

git blame points to this commit: f209562. It changes the layout of classes, introducing two new fields (and removing one). It's not clear from looking at the commit if the field was actually used there; the code hasn't that changed much so I suspect it was already unused as soon as it was introduced, but I don't feel like spending the time necessary to check that properly.

[shindere]

No worries and thanks a lot for the additional bit of context! Who knows, maybe @garrigue will remember, even more than 20 years later!?

[garrigue]

Indeed, I am the one who introduced env_init.
IIRC, the goal of the change was to make creating classes inside functors more efficient. So you will probably need to try examples where a class is defined inside a functor, and a method accesses a value defined itself inside the functor.
At the time this code was written, there was no comprehensive test suite, so it is not really tested by it.

[garrigue]

OK, looking at the code it is clear that env_init has to be stored in the cache, since it is used to build obj_init.
However, I'm no longer sure that it needs to be stored in the class.
Indeed the class is only used either to create new objects (using obj_init) or be reused through inheritance (using class_init). So I do not see a situation where env_init would be used, other than when building obj_init.

This said, objects are not well tested by the testsuite, so only relying on it for correctness is rather risky.
Here is an example of class built in a functor, but you can also do more advanced things, like inheriting inside a functor.

module F (X:sig val x: int end) = struct
  class c = object method x = X.x end
end
module M = F(struct let x = 3 end)

[garrigue]

Independently of the testing, if you remove the explanation of env_init in the class structure, then it should be moved to an explanation about the cache, otherwise there is no explanation left.

[lthls]

I ran a test with functors (extended from the one you provided):

module F (X:sig val x: int end) = struct
  class c = object method x = X.x end
end
module M1 = F(struct let x = 3 end)
module M2 = F(struct let x = 5 end)
let () =
  Format.printf "3 = %d, 5 = %d@." (new M1.c)#x (new M2.c)#x

let alloc_count = ref 0.

module Inherit (Parent : sig class c : object method x : int end end) = struct
  let () = alloc_count := Gc.minor_words ()
  class c = object inherit Parent.c end
  let () = alloc_count := Gc.minor_words () -. !alloc_count
end

module N1 = Inherit(M1)
let () = Format.printf "N1: x = %d, alloc = %d@." (new N1.c)#x (truncate !alloc_count)
module N2 = Inherit(M2)
let () = Format.printf "N2: x = %d, alloc = %d@." (new N2.c)#x (truncate !alloc_count)

The results are as follow:

$ ocamlc -o test test.ml && ./test
3 = 3, 5 = 5
N1: x = 3, alloc = 191
N2: x = 5, alloc = 32
$ ocamlopt -o test test.ml && ./test
3 = 3, 5 = 5
N1: x = 3, alloc = 187
N2: x = 5, alloc = 27

So nothing looks wrong and the optimisation still triggers.

Independently of the testing, if you remove the explanation of env_init in the class structure, then it should be moved to an explanation about the cache, otherwise there is no explanation left.

I wish there was an explanation about the cache somewhere... I'm still not completely sure it works properly.
But I'll see if I can put a comment somewhere.

[lthls]

@garrigue I've added a comment. I would appreciate if you could check that what I'm describing is in line with what you expect the code to do.

[garrigue]

This looks ok now.
I'm still a bit concerned that we don't have that many tests for corner cases, but it really seems that the env_init stored in classes has never been used. The reason for my putting it there is a bit mysterious. It might be a that at some point the class itself was stored in the cache, but I could not find such code.

[garrigue]

takes both the class table to be filled and the environment as parameters

[lthls]

I'm working with @Ekdohibs on documenting the object compilation code, and we will likely produce additional tests to cover the features we document, but this is not our highest priority so it might be a while before we can actually submit a PR.