Thursday, March 2, 2017

create library to override operator()* of iterator - risk dangling pointer

By Hường Hana 1:30 AM , , , Leave a Comment

I am trying to create my own boost::adaptors::transformed.
Here is the related boost code.

Here is its usage (modified from a SO answer by LogicStuff):-

C funcPointer(B& b){      //"funcPointer" is function convert from "B" to "C"     return instance-of-C }  MyArray<B> test;  //<-- any type, must already has begin() & end()  for(C c : test | boost::adaptor::transformed(funcPointer)) {     //... something .... }

The result will be the same as :-

for(auto b : test) {     C c = funcPointer(b);     //... something ... }

My Attempt

I created CollectAdapter that aim to work like boost::adaptor::transformed.
It works OK in most common cases.

Here is the full demo and back up. (same as below code)

The problematic part is CollectAdapter - the core of my library.
I don't know whether I should cache the collection_ by-pointer or by-value.

CollectAdapter encapsulates underlying collection_ (e.g. pointer to std::vector<>) :-

template<class COLLECTION,class ADAPTER>class CollectAdapter{     using CollectAdapterT=CollectAdapter<COLLECTION,ADAPTER>;     COLLECTION* collection_;    //<---- #1  problem? should cache by value?     ADAPTER adapter_;           //<---- = func1 (or func2)     public: CollectAdapter(COLLECTION& collection,ADAPTER adapter){         collection_=&collection;         adapter_=adapter;     }     public: auto begin(){         return IteratorAdapter<             decltype(std::declval<COLLECTION>().begin()),             decltype(adapter_)>             (collection_->begin(),adapter_);     }     public: auto end(){ ..... } };

IteratorAdapter (used above) encapsulates underlying iterator, change behavior of operator* :- 

template<class ITERATORT,class ADAPTER>class IteratorAdapter : public ITERATORT {     ADAPTER adapter_;     public: IteratorAdapter(ITERATORT underlying,ADAPTER adapter) :         ITERATORT(underlying),         adapter_(adapter)     {   }     public: auto operator*(){         return adapter_(ITERATORT::operator*());     } };

CollectAdapterWidget (used below) is just a helper class to construct CollectAdapter-instance.

It can be used like:-

int func1(int i){   return i+10;   } int main(){     std::vector<int> test; test.push_back(5);     for(auto b:CollectAdapterWidget::createAdapter(test,func1)){         //^ create "CollectAdapter<std::vector<int>,func1>" instance          //here, b=5+10=15     } }

Problem

The above code works OK in most cases, except when COLLECTION is a temporary object.

More specifically, dangling pointer potentially occurs when I create adapter of adapter of adapter .... 

int func1(int i){   return i+10;    } int func2(int i){   return i+100;   } template<class T> auto utilityAdapter(const T& t){     auto adapter1=CollectAdapterWidget::createAdapter(t,func1);     auto adapter12=CollectAdapterWidget::createAdapter(adapter1,func2);     //"adapter12.collection_" point to "adapter1"     return adapter12;     //end of scope, "adapter1" is deleted     //"adapter12.collection_" will be dangling pointer } int main(){     std::vector<int> test;     test.push_back(5);     for(auto b:utilityAdapter(test)){         std::cout<< b<<std::endl;   //should 5+10+100 = 115     } }

This will cause run time error. Here is the dangling-pointer demo.

In the real usage, if the interface is more awesome, e.g. use | operator, the bug will be even harder to be detected :- 

//inside "utilityAdapter(t)" return t|func1;        //OK! return t|func1|func2;  //dangling pointer

Question

How to improve my library to fix this error while keeping performance & robustness & maintainablilty near the same level?

In other words, how to cache data or pointer of COLLECTION (that can be adapter or real data-structure) elegantly?

Alternatively, if it is easier to answer by coding from scratch (than modifying my code), go for it. :)

My workarounds

The current code caches by pointer.
The main idea of workarounds is to cache by value instead.

Workaround 1 (always "by value")

Let adapter cache the value of COLLECTION.
Here is the main change:-

COLLECTION collection_;    //<------ #1  //changed from   .... COLLECTION* collection_;

Disadvantage:-

  • Whole data-structure (e.g. std::vector) will be value-copied - waste resource.
    (when use for std::vector directly)

Workaround 2 (two versions of library, best?)

I will create 2 versions of the library - AdapterValue and AdapterPointer.
I have to create related classes (Widget,AdapterIterator,etc.) as well.

  • AdapterValue - by value. (designed for utilityAdapter())
  • AdapterPointer - by pointer. (designed for std::vector)

Disadvantage:-

  • Duplicate code a lot = low maintainability
  • Users (coders) have to be very conscious about which one to pick = low robustness

Workaround 3 (detect type)

I may use template specialization that do this :-

If( COLLECTION is an "CollectAdapter" ){ by value }   Else{ by pointer }

Disadvantage:-

  • Not cooperate well between many adapter classes.
    They have to recognize each other : recognized → cache by value.

Sorry for very long post.

0 Answers

If You Enjoyed This, Take 5 Seconds To Share It

0 comments:

Post a Comment