0

I'm trying to convert all my CopyMemory functions to std::copy functions.

It works with copymemory and memcpy but not std::copy. Can anyone tell me what I'm doing wrong or how to fix it?

template<typename T>
void S(unsigned char* &Destination, const T &Source)
{
    //CopyMemory(Destination, &Source, sizeof(T));
    std::copy(&Source, &Source + sizeof(T), Destination);      //Fails..
    Destination += sizeof(T);
}

template<typename T>
void D(T* &Destination, unsigned char* Source, size_t Size)
{
    //CopyMemory(Destination, Source, Size);
    std::copy(Source, Source + Size, Destination);
    Source += sizeof(T);
}

template<typename T>
void D(T &Destination, unsigned char* Source, size_t Size)
{
    //CopyMemory(&Destination, Source, Size);
    std::copy(Source, Source + Size, &Destination);
    Source += sizeof(T);
}

I've also figured that I could do the following to convert iterators to pointers:

std::string Foo = "fdsgsdgs";

std::string::iterator it = Foo.begin();

unsigned char* pt = &(*it);

How would I convert pointers to iterators then? :S

The code I use to test the memcpy/copymem vs std::copy is as follows (It prints 7 if it works.. and random numbers if it doesn't):

#include <windows.h>
#include <iostream>
#include <vector>
#include <typeinfo>

using namespace std;

typedef struct
{
    int SX, SY;
    uint32_t Stride;
    unsigned long ID;
    int TriangleCount;
} Model;

template<typename T>
void S(unsigned char* &Destination, const T &Source)
{
    CopyMemory(Destination, &Source, sizeof(T));
    Destination += sizeof(T);
}

template<typename T>
void S(unsigned char* &Destination, const std::vector<T> &VectorContainer)
{
    size_t Size = VectorContainer.size();
    for (size_t I = 0; I < Size; ++I)
        S(Destination, VectorContainer[I]);
}

void S(unsigned char* &Destination, const Model &M)
{
    S(Destination, M.SX);
    S(Destination, M.SY);
    S(Destination, M.Stride);
    S(Destination, M.ID);
    S(Destination, M.TriangleCount);
}

template<typename T>
void D(T* &Destination, unsigned char* Source, size_t Size)
{
    CopyMemory(Destination, Source, Size);
    Source += sizeof(T);
}

template<typename T>
void D(T &Destination, unsigned char* Source, size_t Size)
{
    CopyMemory(&Destination, Source, Size);
    Source += sizeof(T);
}

template<typename T>
void D(std::vector<T> &Destination, unsigned char* Source, size_t Size)
{
    Destination.resize(Size);
    for(size_t I = 0; I < Size; ++I)
    {
        D(Destination[I], Source, sizeof(T));
        Source += sizeof(T);
    }
}

void D(Model* &Destination, unsigned char* Source)
{
    D(Destination->SX, Source, sizeof(Destination->SX));
    D(Destination->SY, Source, sizeof(Destination->SY));
    D(Destination->Stride, Source, sizeof(Destination->Stride));
    D(Destination->ID, Source, sizeof(Destination->ID));
    D(Destination->TriangleCount, Source, sizeof(Destination->TriangleCount));
}

long double* LD = new long double[25000];
std::vector<Model> ListOfModels, ListOfData;

void ExecuteCommands()
{
    switch(static_cast<int>(LD[1]))
    {
        case 1:
        {
            LD[2] = 2;
            unsigned char* Data = reinterpret_cast<unsigned char*>(&LD[3]);
            Model M; M.SX = 1; M.SY = 3; M.Stride = 24; M.ID = 7; M.TriangleCount = 9;
            Model K; K.SX = 3; K.SY = 21; K.Stride = 34; K.ID = 9; K.TriangleCount = 28;

            ListOfModels.push_back(M);
            ListOfModels.push_back(K);
            S(Data, ListOfModels);
        }
        break;
    }
}

void* GetData()
{
    unsigned char* Data = reinterpret_cast<unsigned char*>(&LD[3]);
    D(ListOfData, Data, LD[2]);
    cout<<ListOfData[0].ID;        //Should print 7 if it works.
    return &ListOfData[0];
}


int main()
{
    LD[1] = 1;
    ExecuteCommands();
    GetData();
}
Brandon
  • 22,723
  • 11
  • 93
  • 186
  • 1
    [RTFM](http://www.cplusplus.com/reference/algorithm/copy/): Destination has to be an output iterator – Steve-o Sep 21 '12 at 01:40
  • Hmm so I can't use it then since I don't have an iterator :S I guess I'll stick with memcpy. – Brandon Sep 21 '12 at 01:48
  • 2
    What's the error you're getting? The only weird thing I see about you're code (apart from the S's and the D's) is that you templatize either the source type or the destination type, but if the types don't match you'll get a compilation because std::copy is type-safe. – user1610015 Sep 21 '12 at 01:53
  • 4
    @Steve-o Pointers ARE output iterators (unless it's a const pointer) – user1610015 Sep 21 '12 at 01:53
  • I get the random numbers. The code above is compilable in codeblocks if you want to test it. Basically. If I used memcpy, it prints 7. Which is correct. Using std::copy, I get random numbers. If I change all the copymemory calls to std::copy calls, I get: stl_algobase.h|329|error: no match for 'operator=' in '* __result = * __first'| I'm not sure if I passed the parameters to std::copy correctly. – Brandon Sep 21 '12 at 02:08
  • 2
    Suggestions: Describe what you are trying to accomplish as you might be going about it the wrong way, but we cannot tell. Create a minimal representation of the problem that exhibits the symptoms you are seeing. Place the error text at the end of the question or on something like pastebin (and provide a link). – Michael Price Sep 21 '12 at 03:42

1 Answers1

8

There are so many things wrong with this code that it's almost impossible to know where to begin. And the errors are in many cases so basic that it betrays a gross misunderstanding of what you should be doing. The kind of code you're writing is dangerous for experienced C++ programers; the errors you've made in your code suggest that you're far from experienced.

Stop trying to do what you're trying to do.

But let's take your code.

std::copy(&Source, &Source + sizeof(T), Destination);      //Fails..

First, let's talk about pointers in C++.

If you have a pointer to some type T, let's say T *t, doing this t + 1 will not shift the pointer over one byte. This is basic pointer arithmetic stuff here; t + 1 will shift it over by sizeof(T); that's how pointers have worked since the earliest days of C, let alone C++.

Source is a T&, so &Source is a T*. Therefore, adding sizeof(T) to it will increment the pointer by sizeof(T) * sizeof(T). That's not what you want.

Second, std::copy is not memcpy. std::copy is for copying one collection of values (defined by an input iterator pair) into another collection of values defined by an output iterator. std::copy requires that the value_type of the input iterator is implicitly convertible to the value_type of the output iterator.

The value_type of a T*, the input iterator in question, is a T; T*'s point to Ts. The value_type of your char*, your output iterator, is char. std::copy is going to try to do effectively this:

char *val;
T *t;
*val = *t;

Even ignoring the fact that these two pointers are uninitialized, this makes no sense. Unless T has an operator char conversion operator, you cannot simply take a T and shove it into a char. Therefore, you get a compile error. As you should.

If you truly have some T and want to copy it into a char* array of the appropriate size (or vice-versa), std::copy is not the tool you need. The tool you want is std::memcpy. std::copy is for copying objects, not copying bytes.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • Great answer, but I want to add, `std::copy(reinterpret_cast(&t), reinterpret_cast(&t)+sizeof(t), dest)` does work for this purpose. ;) – Xeo Sep 21 '12 at 08:47
  • @Xeo: Everytime I see `reinterpret_cast` my eyes hurt... Besides that, *does work for this purpose* is a very bold statement. I can give you a few types `T` for which it won't work at all (anything that is not a standard layout type actually). Besides that, if you are going to use `memcpy`, just use `memcpy` it will at least be clear in the code that you are doing something potentially dangerous for many types. Writing `memcpy` in terms of `std::copy` does not make it less dangerous and it does make it harder to spot (and potentially slower). – David Rodríguez - dribeas Sep 21 '12 at 12:22
  • @David: Sorry, I thought the ";)" at the end made clear that it was mostly tongue-in-cheek and jokingly. Of course I know that this only creates problems. :) – Xeo Sep 21 '12 at 12:33
  • And why exactly is my code dangerous? It works all the time with memcpy. I understood what you said about the std::copy not working on bytes. The memcpy on the other hand will always work so how is it dangerous? Destination is a char.. I'm not shifting it by 1. It shifts exactly by sizeof(T). I guess you mean I was shifting it wrong in std::copy. – Brandon Sep 21 '12 at 17:04
  • @Cant: "*And why exactly is my code dangerous? It works all the time with memcpy.*" Right up until you try to use it with non-POD structs and things go boom. Or worse, *appear* to work fine, but cause subtle errors that are difficult to track down. There's a reason why C++ has copy constructors. Unless you have some specific *need* to go `memcpy`ing C++ types around, you shouldn't; just use the copy constructor that your C++ class has so nicely given you. – Nicol Bolas Sep 21 '12 at 19:09
  • Can't use the copy constructor. It's for Serialization. S for serialize, D for DeSerialize. I have to serialize all of the above structs and send them to shared memory. Read them and deserialize to reconstruct them on the other side. It works for non-pod types :S Well at least so far it does. – Brandon Sep 21 '12 at 21:25
  • 1
    You cannot treat non-standard layout (non-POD in C++03) types as a series of bytes, period. Try serializing and deserializing a `std::string`. – GManNickG Sep 21 '12 at 23:17
  • 1
    @CantChooseUsernames: If you're doing serialization, you should be using [a *real* serialization library](http://www.boost.org/doc/libs/1_51_0/libs/serialization/doc/index.html), not something based on `memcpy` or other forms of just copying bytes into random memory. The fact that it hasn't broken *yet* doesn't mean that it's not actually broken. – Nicol Bolas Sep 22 '12 at 00:46