A correct way of writing this?

Started by
12 comments, last by irreversible 6 years, 10 months ago

I can't tell if I'm running into a  bug or just don't know how to write this.

Basically I have my own extended VECTOR class derived from std::vector, which is non-copiable. This is by design, because I want to enforce strict rules on when complex objects can be duplicated or how they can be stored. Hence every class that contains a VECTOR is by definition also non-copiable by default.

What I'm writing is an allocator proxy, which is used by the type library during abstract processes like deserialization - it takes a base pointer, casts it to a container type and does stuff with it. It can handle smart pointers and raw objects, but currently has no way of differentiating between the two. Which becomes a problem when a type is non-copiable. This is where things start getting weird.

In Visual Studio I can successfully use the allocator on some complex types (eg types with one or more vector members whose copy assignment and constructor are either not provided), which should not be possible (unless, I guess, VS performs some selective on-demand compilation even in debug mode). In other cases the test fails correctly and the code does not compile.

So I figured, okay - I can just modify the allocator to conditionally compile only the parts that do not handle raw non-copiable objects in containers. All I would have to do is use something like is_copy_constructible<> to  determine which parts get compiled. But no - ALL of the classes I tested this with return true - both classes that contain non-copiable members with no copy constructor defined as well as simple classes that should be trivially copiable. Conversely, is_trivially_copy_constructible<> returns false to ALL of the classes I tried. Okay, then - maybe I'm just too dumb. Besides, the compiler does tell me the offending function is the auto-generated copy assignment operator, not the constructor. So I tried is_copy_assignable<>...

Which doesn't compile:

Error    1    error C2280: 'VECTOR<IActor *> &VECTOR<IActor *>::operator =(const VECTOR<IActor *> &)' : attempting to reference a deleted function   

 

 

I'm confused - maybe I'm completely misinterpreting the docs, but how do I determine whether an object is copiable at compile time? Why does is_copy_assignable<> apparently generate the assignment operator, which then fails to compile? Isn't the entire point of type traits to determine whether a condition is true without altering the behavior of the code in the process? Is this a bug in VS2013, because cppreference.com clearly states: "checks if a type has a copy assignment operator"?

Most importantly - what is a possible solution here?

Advertisement

Extending a standard library container class is almost always the wrong approach.  There are very few things in the standard library designed for extension, and those are clearly indicated.  It is possible to extend some of them, but it usually comes with unexpected ramifications.

Your desire to mix smart pointers and raw pointers, as well as trying to describe what can be copied all reek of a common problem.  Controlling the lifetime of objects is a critical aspect of software, especially true in games. What you describe sounds like a symptom of not understanding what code owns the lifetime.  What is the REAL problem you are trying to solve here? You came up with a solution of trying to do complex things with a vector, but what problem drove you to that solution?

1 hour ago, frob said:

Extending a standard library container class is almost always the wrong approach.  There are very few things in the standard library designed for extension, and those are clearly indicated.  It is possible to extend some of them, but it usually comes with unexpected ramifications.

Your desire to mix smart pointers and raw pointers, as well as trying to describe what can be copied all reek of a common problem.  Controlling the lifetime of objects is a critical aspect of software, especially true in games. What you describe sounds like a symptom of not understanding what code owns the lifetime.  What is the REAL problem you are trying to solve here? You came up with a solution of trying to do complex things with a vector, but what problem drove you to that solution?

I'm doing three things in my overloaded vector class:

1) provide a couple of wrapper functions like quick_erase()
2) disable the copy constructor and copy assignment operator
3) provide a custom allocator

I don't see how any of these violate the extensibility of any standard library class.

I'm not mixing shared and raw pointers. Neither is this particular issue in any way related to controlling lifetimes, although fundamentally it does stem from it. The real problem is exactly the one I described in my post. Given (untested pseudocode):



struct NonCopiable {
	// disables copy semantics
	VECTOR<int32> data;
};
      
template<typename T, bool ISCOPIABLE>
class Allocator {
  public:
    // should always work
	void AllocPointer(void* basePtr)
    	{
      	VECTOR<shared_ptr<T>>* container = reinterpret_cast<VECTOR<shared_ptr<T>>*>(basePtr);
      
     	container->push_back(NewShared<T>());
		}
      
    // should not work with T = NonCopiable. I want to selectively compile this into a stub that throws,
    // indicating an error...
	template <boolean ISCOPIABLE2>
	typename std::enable_if < ISCOPIABLE2, void* >::type
	void DoAllocPlain(void* basePtr)
    	{
      	VECTOR<T>* container = reinterpret_cast<VECTOR<T>*>(basePtr);
      
     	container->push_back(T());
		}
    // ... which means that for T = NonCopiable it should become
	template <boolean ISCOPIABLE2>
	typename std::enable_if <!ISCOPIABLE2, void* >::type      
	void DoAllocPlain(void* basePtr)
    	{
      	throw("You done goofed");
		}  
      
     void AllocPlain() { DoAllocPlain<ISCOPIABLE>(basePtr); }
};
      
...

void LoadToContainer(void *basePtr, int32 contType)
{
      
      
     Allocator<NonCopiable, std::is_copy_assignable<NonCopiable>> allocator;
     ...
     if(contType == ContainerOfSharedPointers)
      	allocator->AllocPointer(basePtr);
     else
      	allocator->AllocPlain(basePtr);
}

It would make no difference if I was using std::vector directly and passing it a plain non-copiable type. It wouldn't compile, resulting in the same situation.

So, to reiterate - my question is about type traits and how to properly employ them to decide which version of AllocPlain() should be compiled.

Edit: added enable_if to the sample code and fixed a few mistakes. PS - code editing in the new forums is apparently exceptionally broken.

std::vector is resizable, which means it needs to be able to copy its data to a new location if it runs out of space, wouldn't it? Not sure how you can store non-copiable elements in it, my guess is not.

Resizability also means the actual vector data is not the container<> object, as far as I kcan see. If the actual vector data was in the container<> object, it could move if you add an element and it needs resizing, and the STL API doesn't say anything about that. Therefore, your allocation is perhaps not even allocating what you think you're allocating?

Instead of fighting the STL, why not make your own wrapper class around a new-ed T[] array? That would be much simpler to get working, I suspect.

27 minutes ago, Alberth said:

std::vector is resizable, which means it needs to be able to copy its data to a new location if it runs out of space, wouldn't it? Not sure how you can store non-copiable elements in it, my guess is not.

Resizability also means the actual vector data is not the container<> object, as far as I kcan see. If the actual vector data was in the container<> object, it could move if you add an element and it needs resizing, and the STL API doesn't say anything about that. Therefore, your allocation is perhaps not even allocating what you think you're allocating?

Instead of fighting the STL, why not make your own wrapper class around a new-ed T[] array? That would be much simpler to get working, I suspect.

I'm just going to assume that I'm extremely poor at communicating my problem at this point :)

This is exactly the thing I'm trying to not to do. I want to automatically NOT generate code in a generic class that would handle non-copiables in a container, throwing instead if I accidentally I do end up trying to do so. 

I feel like a really bad communicator here. How could I further clarify my question?

1 hour ago, irreversible said:

2) disable the copy constructor and copy assignment operator

A derived class can never renege on features provided by the base class; doing so is an violation of OO's LSP rule, so this approach is invalid. OO would force you to use composition instead of inheritance here. Make your own class from scratch which contains a vector within the private implementation details.

2 hours ago, Hodgman said:

A derived class can never renege on features provided by the base class; doing so is an violation of OO's LSP rule, so this approach is invalid. OO would force you to use composition instead of inheritance here. Make your own class from scratch which contains a vector within the private implementation details.

Fair enough - that makes sense. Thanks for the heads up, although I'm somewhat curious how this becomes invalid in practice so long as I use the derived class - the compiler doesn't seem to be bothered by it.

Also, that being said, why does the below code generate a deleted function compile time error for both is_copy_assignable and is_trivially_copy_assignable:

error C2280: 'myvec<int32> myvec<int32>::operator =(const myvec<int32> &)' : attempting to reference a deleted function  

is_trivially_copy_constructible yields false and is_copy_constructible yields true.

My expectation would be that all of these cases would a) compile and b) yield false on the account of my explicitly disabling the copy semantics. As mentioned above, I'm using VS2013.
 


template<typename T>
class myvec {
    private:
        std::vector<T> detail;

    public:
  		myvec() { }
  
        myvec<T> operator=(IN const myvec<T>& copy) = delete;
        myvec(IN const myvec<T>& copy) = delete;
};

  
class NonCopiableA {
        myvec<int32>	vec;
};

  
... (DOUTEX() is a debug output macro) ...
  
    DOUTEX(std::is_copy_assignable<NonCopiableA>::value);
    DOUTEX(std::is_trivially_copy_assignable<NonCopiableA>::value);
    DOUTEX(std::is_copy_constructible<NonCopiableA>::value);
    DOUTEX(std::is_trivially_copy_constructible<NonCopiableA>::value);
5 minutes ago, irreversible said:

Also, that being said, why does the below code generate a deleted function compile time error for both is_copy_assignable and is_trivially_copy_assignable:

Nowadays, C++ compilers seem pretty much ok with having implementations only for code that you actually use. My g++ happily compiles plain classes that have unused method declarations without any implementation. In the past it would throw "invalid v-table" errors during linking.

My guess is that your test-case doesn't use all functionality, and only hits the error when you access a method that uses the deleted functionality.

15 minutes ago, Alberth said:

Nowadays, C++ compilers seem pretty much ok with having implementations only for code that you actually use. My g++ happily compiles plain classes that have unused method declarations without any implementation. In the past it would throw "invalid v-table" errors during linking.

My guess is that your test-case doesn't use all functionality, and only hits the error when you access a method that uses the deleted functionality.

The problem is that in the above test case I don't see where the code would be used :). I'm not omitting anything.

28 minutes ago, irreversible said:

I'm somewhat curious how this becomes invalid in practice so long as I use the derived class - the compiler doesn't seem to be bothered by it.

It's a violation of OO theory, not the OOP constructs of C++ in practice. OO theory says that any algorithm that works on a base class, must also work on all derived types. You can break these rules in theory, but that means that you can't really claim to be writing "OO" code, even though you're using OOP language features...

Another C++-specific solution would be to use private (implementation) inheritance, which sidesteps the theoretical rules that come with public (interface) inheritance, as implementation inheritance doesn't exist in the theory.

28 minutes ago, irreversible said:

Also, that being said, why does the below code generate a deleted function compile time error for both is_copy_assignable and is_trivially_copy_assignable:

That is confusing... however it doesn't happen for me on MSVC2015... Also I get:
std::is_copy_assignable<NonCopiableA>::value == false
std::is_trivially_copy_assignable<NonCopiableA>::value == false
std::is_copy_constructible<NonCopiableA>::value == false
std::is_trivially_copy_constructible<NonCopiableA>::value == false

Can you post a compilable example and note the line that causes the compile error?

This topic is closed to new replies.

Advertisement