I want to declare a pointer of TGraph, TFile, Three and TH1D and use new method for them inside a for loop, at the end of one loop I delete them so that I can reuse them at the next loop. Here is what I wrote:
int main(){
TGraph* g;
TFile* f;
TH1D* h;
for(some loop conditions){
g = new TGraph();
h = new TH1D();
f = new TFile("some.root");
TTree* t = (TTree*)f->Get("tree");
//do something here
delete g;
delete f;
delete h;
delete t;
}
return 0;
}
But I got error when finished the very first loop:
MakeGainCurve(90527,0x100f88580) malloc: *** error for object 0x121f82580: pointer being freed was not allocated
MakeGainCurve(90527,0x100f88580) malloc: *** set a breakpoint in malloc_error_break to debug
I cannot stress this enough, please refrain absolutely from implementing your code with the pattern you show. No new or delete statements should be visible in user code in modern C++ applications. Try to reframe your application so that ideally you don’t need raw pointers, or if you strictly need them then the lifetime and ownership of objects should be controlled and understandable. Let me give you an example
TTree *myTree;
for (condition){
std::unique_ptr<TFile> f{TFile::Open("myfile.root")};
myTree = f->Get<TTree>("myTree");
// use myTree in the loop
// CRUCIAL: test the raw pointer
if(myTree){
// use it after the check, otherwise you could have a nullptr
}
// Anything that is created manually inside the loop, shouldn't be created with `new`
TGraph myGraph{};
TH1D myTH1D{};
}
In the example above, it is clear that TTree * is just a view on the tree owned by the TFile. Since it is wrapped by a unique_ptr the management will be done automatically and both the tree and file will be deleted appropriately at the end of the scope.
Thank you for the explanation! Just curious about the example you showed, I can use a raw pointer of TTree because the memory it pointing is owned by the TFile and will be released after every loop?
If I want to use a unique_ptr for TTree inside the for loop just the same as TFile, how should I write? Because Get() function will return a raw pointer so I don’t know how to cast it to a unique_ptr.
Yes, you can surely use unique_ptr everywhere (and I highly suggest you to do so!)
for (...){
std::unique_ptr<TFile> f{TFile::Open("myfile.root")};
// Pointers should be always checked to avoid use of nullptr
if (!f){
// The file is not properly opened, do something about it
throw std::runtime_error();
}
std::unique_ptr<TTree> myTree{f->Get<TTree>("myTree")};
if (!myTree){
// The tree was not properly extracted from the file, do something about it
throw std::runtime_error();
}
// Anything that is created manually inside the loop, shouldn't be created with `new`
TGraph myGraph{};
TH1D myTH1D{};
}
In general, whenever a function returns a T * which ownership is given to you (the caller), just wrap it with a unique_ptr as shown above. This is usually the case for legacy functions that were written before C++11.
Thank you @vpadulan !
I understood how to use unique_ptr and will try to use it
Can I just ask one question for curiosity, if I use the old school style of raw pointers for objects (just like my original question), what is the correct way to use them? I mean I thought that was the correct way:
create new pointers outside the loop and new at the begin of every loops then delete them at the end of every loops so that I can new them again at the exit loop.
Sorry because this may not related to ROOT but to c++ itself…
What @yus suggests is true, in fact you are not creating your TTree *t with a new. Instead you are getting it as a result from TFile::Get. The TFile is the owner of the TTree, so if you delete it manually like you are doing, then the TFile will delete it again when going out of scope, and that will trigger a double delete.
