current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I need to make a vector of different types and be able to delete the values (please ignore anything else but deleting the values for now). Is the next code safe to delete the values?

#include <memory>
#include <vector>
#include <string>

using MyPtr = std::unique_ptr<void, void(*)(void*)>;

template<typename T>
MyPtr MakePtr(const T& val)
{
    return MyPtr(new T(val), [](void* ptr){delete static_cast<T*>(ptr);});
}

int main()
{
    std::vector<MyPtr> ptrs;

    ptrs.push_back(MakePtr(1));
    ptrs.push_back(MakePtr(1.1));
    ptrs.push_back(MakePtr(1.f));
    ptrs.push_back(MakePtr(std::string("1")));

    return 0;
}
share|improve this question
    
Might be more appropriate at stackoverflow.com? –  CashCow Oct 20 '14 at 10:06
    
There's not enough context to review this. Whatever you are trying to do, this is probably not the way to do it. –  Yuushi Oct 20 '14 at 10:14

2 Answers 2

up vote 1 down vote accepted

You have asked for a "review" but really asked whether your deleter is safe or not.

Whilst the deleter looks to be technically not an issue as you can go from your pointer type to void* and back and should get to where you started, it doesn't look right to create your variant through unique_ptr<void> in which you are not storing any type-info so how exactly do you get the values out as the values?

Variants in C++: Yes I have seen loads of implementations. I have made my own too. Even boost contains two. We would need to know really more context of what types you might store, and how you need to extract them.

The reason there are so many is that often you are going to have lots of them, carrying lots of data about, and they need to work efficiently for your own situation.

share|improve this answer
up vote 3 down vote

Is the next code safe to delete the values?

No. At the very least, you should probably use reinterpret_cast instead of static_cast.Yes.

That said, it seems you are having the x-y problem. Storing data in C++, when you have heterogenous types in a sequence, should be done using a variant type, not void*; in other words, you should create a system that stores your type information per type/variable instance, instead of discarding all type information - like void* does). You are better off maintaining sepatate element lists (sparse arrays or some kind) by type, than you are with a vector<void*> (or vector<unique_ptr<void>> for that matter).

Have a look at boost::any and boost::variant before implementing your solution. You could use one of those, or a separate (similar) implementation.

share|improve this answer
    
Why do you say I should use reinterpret_cast instead of static_cast? –  Felics Oct 20 '14 at 11:32
    
I don't see that reinterpret_cast will be any different / better here although I am not sure if the user is asking if his deleter is safe or whether he should make his own variant from unique_ptr<void> –  CashCow Oct 20 '14 at 11:37
    
@CashCow I'm just asking if my deleter is safe. –  Felics Oct 20 '14 at 11:37
    
@Felics, you shouldn't - my bad (I have edited my response). –  utnapistim Oct 20 '14 at 11:51
1  
@CashCow: I prefer reinterpret_cast<> in this situation. It should work the same as static_cast<> but it sticks out more. So you have a harder look a reinterpret_cast<> to make sure they are correct. People have a tendency (incorrectly) to think that static_cast<> is safer than reinterpret_cast<> I want them to see this situation as the most dangerous way they can think of it so that they put real eyeballs on the code and scrutinize it. –  Loki Astari Oct 20 '14 at 19:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.