Bug in TObjArray->Remove(pointer)

[Birdsall]

ROOT Version: 6.18/04
Built for linuxx8664gcc on Sep 11 2019, 15:38:23
From tags/v6-18-04@v6-18-04

Fairly sure this is a bug. I have a TObjArray containing several histograms. (TH1)

Some associated code…

        TObject *tmpobj_P1 = allDataSamples1D->FindObject(search_object_P1.c_str());
        TObject *tmpobj_P2 = allDataSamples1D->FindObject(search_object_P2.c_str());
        if(tmpobj_P1 == nullptr)
        {
            std::cout << "could not find object: " << search_object_P1 << std::endl;
            /*for(int i = 0; i < allDataSamples1D->GetEntries(); ++ i)
            {
                std::cout << allDataSamples1D->At(i)->GetName() << std::endl;
            }*/
            throw "problem";
        }
        if(tmpobj_P2 == nullptr)
        {
            std::cout << "could not find object: " << search_object_P2 << std::endl;
            throw "problem";
        }

        if(debuglevel >= 5)
        {
            std::cout << "before ->Remove():" << std::endl;
            for(int i = 0; i < allDataSamples1D->GetEntries(); ++ i)
            {
                std::cout << "allDataSamples1D->At(" << i << ")=" << allDataSamples1D->At(i) << std::endl;
                std::cout << allDataSamples1D->At(i)->GetName() << std::endl;
            }
        }
        // NOTE: this just removes the object from the TObjArray
        // it does not delete the object from gROOT memory
        if(allDataSamples1D->Remove(tmpobj_P1) == nullptr)
        {
            std::cout << "FAILED TO REMOVE" << std::endl;
            throw "failed to remove";
        }
        if(allDataSamples1D->Remove(tmpobj_P2) == nullptr)
        {
            std::cout << "FAILED TO REMOVE" << std::endl;
            throw "failed to remove";
        }
        if(debuglevel >= 5)
        {
            std::cout << "after ->Remove():" << std::endl;
            std::cout << "there are " << allDataSamples1D->GetEntries() << " samples to search" << std::endl;
            for(int i = 0; i < allDataSamples1D->GetEntries(); ++ i)
            {
                std::cout << "allDataSamples1D->At(" << i << ")=" << allDataSamples1D->At(i) << std::endl;
                //std::cout << allDataSamples1D->At(i)->GetName() << std::endl;
            }
        }

Here is the associated output

before ->Remove():
allDataSamples1D->At(0)=0x590c690
hTotalE_data_2e_P1
allDataSamples1D->At(1)=0x571a210
hTotalE_data_2e_P2
allDataSamples1D->At(2)=0x5452e90
hSingleEnergy_data_2e_P1
allDataSamples1D->At(3)=0x55a8640
hSingleEnergy_data_2e_P2
allDataSamples1D->At(4)=0x57a8af0
hHighEnergy_data_2e_P1
allDataSamples1D->At(5)=0x57baab0
hHighEnergy_data_2e_P2
allDataSamples1D->At(6)=0x5a96750
hLowEnergy_data_2e_P1
allDataSamples1D->At(7)=0x5aa7530
hLowEnergy_data_2e_P2
allDataSamples1D->At(8)=0x573a370
hEnergySum_data_2e_P1
allDataSamples1D->At(9)=0x5f865e0
hEnergySum_data_2e_P2
allDataSamples1D->At(10)=0x564a800
hEnergyDiff_data_2e_P1
allDataSamples1D->At(11)=0x65dfe30
hEnergyDiff_data_2e_P2
after ->Remove():
there are 10 samples to search
allDataSamples1D->At(0)=0
allDataSamples1D->At(1)=0
allDataSamples1D->At(2)=0x5452e90
allDataSamples1D->At(3)=0x55a8640
allDataSamples1D->At(4)=0x57a8af0
allDataSamples1D->At(5)=0x57baab0
allDataSamples1D->At(6)=0x5a96750
allDataSamples1D->At(7)=0x5aa7530
allDataSamples1D->At(8)=0x573a370
allDataSamples1D->At(9)=0x5f865e0

You can see that the pointers 0x590c690 and 0x571a210 are found from the calls to FindObject.

The places in the TObjArray where these were (index 0 and 1 respectively) are set to null (0).

Then two objects (pointers) are popped off the end of the array from index 10 and 11, leaving 10 items remaining instead of 12 (which is what the array contained before calling Remove).

I can find a work around for this, so it’s not immediatly a problem for me personally but it should probably be fixed.

[pcanal]

This is the intended behavior. Related TClonesArray::GetEntries returns the number of active elements while TClonesArray::GetEntriesFast returns the highest non-empty slot in the array +1 (i.e. a number useable for iteration).

To get the behavior you expect, you can call:

allDataSamples1D->Compress();

after the Remove(s) have been done.

[Birdsall]

Ok thanks for this.

So it is the case that Remove() “removes an element by setting it to zero”, and decreases the “size” by 1? But it does not align the data so that calls to At() are valid?

Is this correct? If so why - this seems like an unusual behaviour for a container. I am assuming there is some other logical reason for why this design was intended?

[pcanal]

TClonesArray is designed to reduce/minimize memory allocation and memory copy. So each slot is associated with a object and technically Remove() actually set aside the object for later re-use and (temporarily) set it slot value to nullptr. When re-using this slot, rather than allocating a new object, it will reuse the previously allocated memory. In an array like container (i.e. TClonesArray, TObjArray and std::vector), ‘erasing’ an element (i.e the std::vector behavior) can be very costly (for large object or large containers) as it involved moving in memory all the subsequent elements.

[Birdsall]

Right ok, makes complete sense. Might be worth making an update to the documentation to make this clear however? At least as far as I am aware there is nothing in the current online manual about this - but I might be mistaken on that point.