FieldVerifier::eliminators has memory leak

[mo-xiaoming]

FieldVerifier::FieldVerifier() {
  this->eliminators.push_back(new HFRecordEliminator());
  this->eliminators.push_back(new HFSLookupEliminator());
  this->eliminators.push_back(new HFLookupEliminator());
  this->eliminators.push_back(new HFTEnvLookupEliminator());
}

Those memory are never got released. Those pointers cannot be simply replaced by smart pointers. Because some of pointers in eliminator might not be dynamically allocated. There is another API for adding,

void FieldVerifier::addEliminator(HFEliminator* hfe) {
  this->eliminators.push_back(hfe);
  ...
}

it is called by

ProcessP::ProcessO(FieldVerifier* fv) {
  fv->addEliminator(&this->procman);
}

and procman is not dynamically allocated

class ProcessP : public Unqualifier {
...
  ProcManager procman;
};

My first try was to use concrete member variables as member variables, and pass their pointers to eliminator. However, theirs corresponding headers have to be included in FieldVerifier.H, which causes recursive header include error.

To make things worse, CtorVerifier, AppendsToUnqualifier etc. are all using the same pattern.

The easiest (improper?) way I can think of is to keep current API and do it like this

struct HFEliminator {
...
private:
   using HFEliminators = std::vector<HFEliminator*>;
   HFEliminators eliminators;

   std::shared_ptr<HFEliminator> hfRecordEliminator;
   std::shared_ptr<HFEliminator> hfsLookupEliminator;
   std::shared_ptr<HFEliminator> hfLookupEliminator;
   std::shared_ptr<HFEliminator> hftEnvLookupEliminator;
};

FieldVerifier::FieldVerifier() {    
  hfRecordEliminator = std::make_shared<HFRecordEliminator>();    
  hfsLookupEliminator = std::make_shared<HFSLookupEliminator>();    
  hfLookupEliminator = std::make_shared<HFLookupEliminator>();    
  hftEnvLookupEliminator = std::make_shared<HFTEnvLookupEliminator>(); 

  this->eliminators.push_back(hfRecordEliminator.get());    
  this->eliminators.push_back(hfsLookupEliminator.get());    
  this->eliminators.push_back(hfLookupEliminator.get());    
  this->eliminators.push_back(hftEnvLookupEliminator.get());  
} 

It indeed fixes the memory leak (confirmed by valgrind), however, it feels hacky.

I'll try to figure out a better way and create a PR in the next few days.

[mo-xiaoming]

I was wrong, it mixes owning and non-owning pointers all over the place

void initStorageFileDef(FieldVerifier* fv, cc&c) {
...
  fv->addEliminator(new DBFieldLookup());
...
}

API probably has to be changed somehow, It's going to take more than few days for sure

[mo-xiaoming]

407 should fix this kind of leak

valgrind --leak-check=full --track-origins=yes ./hobbes-test --tests Matching

before

==55628==    definitely lost: 133,583 bytes in 1,451 blocks
==55628==    indirectly lost: 5,533,555 bytes in 91,433 blocks
==55628==      possibly lost: 1,224 bytes in 11 blocks
==55628==    still reachable: 24,596,810 bytes in 316,205 blocks
==55628==         suppressed: 0 bytes in 0 blocks

after

==16350==    definitely lost: 132,623 bytes in 1,427 blocks
==16350==    indirectly lost: 5,471,760 bytes in 90,212 blocks
==16350==      possibly lost: 112 bytes in 1 blocks
==16350==    still reachable: 24,486,706 bytes in 314,775 blocks
==16350==         suppressed: 0 bytes in 0 blocks

From valgrind log, there are no obvious addEliminator related memory leaks