Tcl Source Code

1. What is the change to tclBasic.c doing in there?
2. The RemoveItem macro probably ought to be written like this:

   #define RemoveItem(type, lst, i) \
       do { \
   	Remove ## type ((lst).list, (lst).num, i); \
   	(lst).num--; \
       } while (0)
   

   It's a standard transform that defangs the worst of syntactic games that C can throw at you.
3. The changes to use TclNRCreateCommandInNs instead of directly making the my command must be performance-profiled. It's because of performance profiling that the weird direct method was used at all; that code is hot-path for object instantiation and we want to try to avoid slowing that down.
4. I like that it looks like some simplifications have been possible in the deletion code. I really really wish that I could be certain that they were correct from just inspection. There's a great many ways an object can get deleted, such as by its destroy method, by rename, by having its namespace wiped out, by having its interpreter destroyed, and by having its class or mixins destroyed. And maybe others.
5. Re the comment:

   To do: Get dkf to weigh in on wether this should be protected with a !IsRoot() condition.

   the issue is that oo::object and oo::class point mutually at each other (and are the only pair of classes where this is legal). Care must be taken to ensure that the specialness is not undone by accident and the usual consequences of trying probably should be either a “computer says no!” or an orderly delete of the whole of TclOO.
6. The change to tclOOBasic.c is wrong; the destroy method should be ensuring that no extra arguments are passed, as it is documented to not take extra args.
7. I'm not sure about the changes to the FOREACH macro; it was intended to be a simple iterator rather than providing those extra skip-NULL-entry capabilities, so adding extra checking such as you do runs the risk of introducing hazards.

Merged into core-8-branch in [edf6105464].

This code fixes [2713bfcb017b].

Should the call to TclNRCreateCommandInNs be replaced by the manual creation of the command, and is this code otherwise good to be merged into the release branches?

Using this script for profiling,

time {::apply {{} {rename [::oo::object new] {}}} 500000

It's difficult to see a difference that rises above the noise, but it looks like the TclNRCreateCommandInNs variant may be a couple of tenths of a microsecond slower, e.g. 30.6 vs 30.4.

dkf, thank you for the quick turnaround on the request for review.

I've removed the line in tclBasic.c, which was a mistake, and modified the RemoveItem macro as suggested.

In AllocObject, the note next to the previous directly-constructed command indicated that the performance issue lay in string manipulation. TclNRCreateCommandInNs(), which has only come available recently, should obviate that cost. I will add profile information shortly.

I reviewed all the paths that could result in deletion of the object. Actually, harmonization of those paths was the goal of this effort.

The new DeleteDesendants function and the additional reference counting may make the IsRoot() check unnecessary. Maybe it can be left out until there is a test that indicates the need for it.

I've added the check for arguments to the destructor back into tclOOBasic.c

The new behaviour of FOREACH may indeed be unneccessary. It was added at the beginning of the work, and the work evolved in a direction that should preclude the existence of any NULL entries in these arrays. However, this introduces a new performance cost in RemoveClass and RemoveObject, which now iterate over the array in order to remove the NULL entry. I will investigate this further while profiling.

Link to [2201fdb82c].