• Advertisement
Sign in to follow this  

My Memcopy

This topic is 4260 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I was wanting to write my own memcopy function and am pretty sure I got it right. Any of you out there see anything that may be wrong with this? void* memcpy( void* dest, void* src, size_t size ) { byte* pDest = (byte*)dest; byte* pSrc = (byte*)src; assert( dest != NULL && src != NULL ); while( size-- > 0 ) *pDest++ = *pSrc++; return (pDest); }

Share this post


Link to post
Share on other sites
Advertisement
Guest Anonymous Poster
Quote:
Original post by E-Lo
byte* pDest = (byte*)dest;
byte* pSrc = (byte*)src;


What's a byte? (Hint: unsigned char)
If this is C, why are you casting? (Hint: it's not needed)

Share this post


Link to post
Share on other sites
not C, but C++...not really sure if that makes a diff

Share this post


Link to post
Share on other sites
And why re-invent the wheel when better systems exist?

I'd assert before you do the byte * pDest = (byte *)dest;, why waste time when its not going to work?

Share this post


Link to post
Share on other sites
If you're trying to implement the C standard library memcpy() function, you got the return value wrong.

Share this post


Link to post
Share on other sites
Quote:
Original post by SiCrane
If you're trying to implement the C standard library memcpy() function, you got the return value wrong.


How is that so? Isn't pDest the pointer to where it was moved, which is what I would want to know?

Share this post


Link to post
Share on other sites
Quote:
Original post by E-Lo
Quote:
Original post by SiCrane
If you're trying to implement the C standard library memcpy() function, you got the return value wrong.


How is that so? Isn't pDest the pointer to where it was moved, which is what I would want to know?


pDest now points to the last byte of your copied range.

You should return dest

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by Mike2343
And why re-invent the wheel when better systems exist?

I'd assert before you do the byte * pDest = (byte *)dest;, why waste time when its not going to work?


If it were C, then you couldn't do it in that order. In fact, the assert() isn't required by either C or C++, why waste time on the comparison? It won't even catch the "overlapping objects" bug.

Share this post


Link to post
Share on other sites
Id also make use of 4 byte movements instead of single byte copying

Share this post


Link to post
Share on other sites
In C++ you shouldn't even use memcpy or memmove, but instead std::copy from the <algorithm> header. It will call constructors when it has to, and generally will optimize down to the same executable code as memcpy or memmove (as appropriate) when possible (i.e. if this seems not to be the case for you, you should upgrade your compiler).

Share this post


Link to post
Share on other sites
converting the data into integers should go a lot faster than converting it into bytes, though you will have to check if the data is a multiple of 4 and handle it accordingly.

Share this post


Link to post
Share on other sites
I believe I am right in saying that the compiler vendor's memcpy may also take advantage of underlying hardware instructions to speed up the copy and be implemented in assembly so you better have a very good reason for writing your own.

If it is purely as an exercise, you would be best off implementing it as simply and reliably as possible rather than as efficiently as possible since you will never compete with the vendor's version for speed or efficiency.

Share this post


Link to post
Share on other sites
I know the MSVC version of memcpy copies on aligned addresses, copying 4 bytes at a time (All in assembly)

Share this post


Link to post
Share on other sites
Quote:
Original post by Anonymous Poster
Quote:
Original post by Mike2343
And why re-invent the wheel when better systems exist?

I'd assert before you do the byte * pDest = (byte *)dest;, why waste time when its not going to work?


If it were C, then you couldn't do it in that order. In fact, the assert() isn't required by either C or C++, why waste time on the comparison? It won't even catch the "overlapping objects" bug.


1) the "why waste time" argument is really not needed here - this implementation of memcpy ir rather naive (no offense intended) and I believe that it is more a learning tool than production code.

2) since assert() will vanish when the code is compiled in release mode, I don't see anything wrong with it.

3) the OP has been successfully Zahlmanized to std::copy() so I don't need to add anything else [smile]

Regards,

Share this post


Link to post
Share on other sites
You can easily make the code more efficient in VS2005 by doing:

#if defined(_MSC_VER) && _MSC_VER >= 1400
# define RESTRICT __restrict
#else
# define RESTRICT
#endif

void* memcpy( void* RESTRICT dest, void* RESTRICT src, size_t size )
{
byte* pDest = (byte*)dest;
byte* pSrc = (byte*)src;

assert( dest != NULL && src != NULL );

while( size-- > 0 )
*pDest++ = *pSrc++;

return (pDest);
}

and then the whole loop gets optimised into a call to the standard library's memcpy function [smile]

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by Emmanuel Deloget
Quote:
Original post by Anonymous Poster
Quote:
Original post by Mike2343
And why re-invent the wheel when better systems exist?

I'd assert before you do the byte * pDest = (byte *)dest;, why waste time when its not going to work?


If it were C, then you couldn't do it in that order. In fact, the assert() isn't required by either C or C++, why waste time on the comparison? It won't even catch the "overlapping objects" bug.


1) the "why waste time" argument is really not needed here - this implementation of memcpy ir rather naive (no offense intended) and I believe that it is more a learning tool than production code.


Mike brought up wasting time.

Quote:

2) since assert() will vanish when the code is compiled in release mode, I don't see anything wrong with it.


My point was more that another assert should be added, one that checks for a more serious bug (writing to or reading from NULL will probably crash the program anyway).

Quote:

3) the OP has been successfully Zahlmanized to std::copy() so I don't need to add anything else


Which means I think Zahlman missed the point (or purposefully ignored it). It's obviously an exercise.

Share this post


Link to post
Share on other sites
Quote:
Original post by Anonymous Poster
Quote:

3) the OP has been successfully Zahlmanized to std::copy() so I don't need to add anything else


Which means I think Zahlman missed the point (or purposefully ignored it). It's obviously an exercise.


Yep. TBH, I don't generally think very highly of these sorts of exercises.

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
TBH, I don't generally think very highly of these sorts of exercises.


I tend to agree with this, I've learnt more by driving into more complex systems having read about them and solved the bugs than you are ever going to learn by writing your own memory copy function (or many other things).

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by Zahlman
In C++ you shouldn't even use memcpy or memmove, but instead std::copy from the <algorithm> header. It will call constructors when it has to, and generally will optimize down to the same executable code as memcpy or memmove (as appropriate) when possible (i.e. if this seems not to be the case for you, you should upgrade your compiler).


I was under the impression that std::copy works only on STL containers (or custom types with Iterator support), the docs (http://www.sgi.com/tech/stl/copy.html) certainly seem to imply it. Are you saying std::copy will work for any arbitrary type?

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by Anonymous Poster
Quote:
Original post by Zahlman
In C++ you shouldn't even use memcpy or memmove, but instead std::copy from the <algorithm> header. It will call constructors when it has to, and generally will optimize down to the same executable code as memcpy or memmove (as appropriate) when possible (i.e. if this seems not to be the case for you, you should upgrade your compiler).


I was under the impression that std::copy works only on STL containers (or custom types with Iterator support), the docs (http://www.sgi.com/tech/stl/copy.html) certainly seem to imply it. Are you saying std::copy will work for any arbitrary type?


Yep, Just use a pointer.

Share this post


Link to post
Share on other sites
I brought up wasting time yes but more like phantom said. Learn from looking at more complext systems and studying them. A memcopy is pretty basic and really not needed except in very specific cases and those are likely not game related (cannot think of one at this time anyways).

Checking for NULL isn't a bad thing, crashing is. I rather have it assert out in debug mode then just plain crash later and not know where or why. Asserts cost nothing in release mode, so they're never really bad to use if used properly.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement