6
struct Something
{
    int a;
    int b;

    Something(char* buffer)
    {
        memcpy(this, buffer, sizeof(Something));
    };
};

Is this legal? safe? to me it looks fine but I'm not sure if the C++ standard prohibits it somehow.

Gam
  • 1,254
  • 1
  • 9
  • 18
  • Possible duplicate of [Why isn't sizeof for a struct equal to the sum of sizeof of each member?](https://stackoverflow.com/questions/119123/why-isnt-sizeof-for-a-struct-equal-to-the-sum-of-sizeof-of-each-member) – Hatted Rooster Jun 25 '17 at 14:10
  • 1
    @GillBates That's not what I'm asking. – Gam Jun 25 '17 at 14:11
  • ***safe?*** It's only safe if the class is a POD type. – drescherjm Jun 25 '17 at 14:11
  • 1
    @Gill I don't see why that is relevant. –  Jun 25 '17 at 14:11
  • @drescherjm Yeah I knew it was safe if it was a POD class, but since I added the constructor it's no longer, hence the question. – Gam Jun 25 '17 at 14:12
  • Because that static_assert is why I marked it in the first place. But I misread that that is not the source of the question. – Hatted Rooster Jun 25 '17 at 14:13
  • It's trivial to turn this into an actual POD type anyway – harold Jun 25 '17 at 14:15
  • @harold yes it is, but I wanted to have the constructor to do some assertions in debug mode (to make sure the data read directly from the buffer was always in the ranges defined by my application) – Gam Jun 25 '17 at 14:22
  • @Phantom: Those assertions can be placed inside a free function which does the copy, too. (Since a constructor is nothing but a free function which also gets a this pointer). The class can then be a POD. – Pixelchemist Jun 25 '17 at 14:25
  • 4
    In any code which actually matters relying on `memcpy()` and the like will become a legacy causing massive portability problems. While I think the code is OK, I'd strongly recommend against it: I'm working with a large code base which has a lot of hard to locate assumption around the layout and size of objects. I'm sure the code started out with "this will never be used other than sending messages within a process" which then became sending messages to other processes, other nodes in a network, etc. – Dietmar Kühl Jun 25 '17 at 14:26
  • @DietmarKühl That's that stable thing that has been staying with c++ the last 30 years. Always was a pleasure to read your useful input ;-) (you may remember me from the good ole usenet times) – πάντα ῥεῖ Jun 25 '17 at 14:39
  • @DietmarKühl Isn't this an exception? this is a small compact message class, fixing the problem in one place will fix all the other problems. As I previously said, I want a constructor to do asserts on the data, and I also have tests for the message class (which consists of creating a message, placing it in a buffer, and reading the message back from the buffer, and then comparing all the values to make sure they remain the same). – Gam Jun 25 '17 at 15:07
  • @Phantom: traveling in terms of bytes instead of stronger typed entities hides the use and prevents the compiler from picking up corresponding uses. As I said above: any code which matters will live on and get extended. Making it hard to see through what is going on will become a legacy problem. Effectively it is hiding information from readers of the code (remember that a future you may be a reader of this code). The exceptions where use of `memcpy()` is kind of OK is when they locally do the work. However, the bytes passed into the constructor are clearly non-local. – Dietmar Kühl Jun 25 '17 at 15:13

2 Answers2

0

... from the fact that it's no longer a POD type after I added the constructor.

That's no fact (merely fake news ;-) ). Adding a constructor doesn't change a structs POD type status.

You can also easily check this with a static_assert:

static_assert( "Something must be a POD type!",std::is_pod(Something)::value);

Is this legal?

I'm not so sure. Depends on the context. In your's it would work and compile without errors or warnings like intended

safe?

Certainly not.

It calls undefined behavior in various ways.

  1. this may consist of more than just the data members. There maybe things like a vtable held.
  2. The compiler is allowed to change the memory layout of the member variables. So padding might occur.
  3. In cause the data is interchanged via a network, endianess comes into play, and has to be considered during de-/serialization

You should note, that any kind of reinterpret_cast (i.e. c-style cast) is giving you undefined behavior to some degree. You need 100% to know what you're doing, and I recommend to check the emitted assembly output and memory layout every time when you use such constructs.

πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190
  • 4
    1) There is no vtable in this case, 2) In that case you also copy the padding. –  Jun 25 '17 at 14:14
  • @Neil I have my doubts to recommend such though (in general). Technically it's UB. – πάντα ῥεῖ Jun 25 '17 at 14:16
  • I wouldn't recommend it myself,. But I'm not sure its illegal. –  Jun 25 '17 at 14:17
  • To be clear, this is not a virtual class and will never be. It was meant to be used as a quick way to send a message to another thread in the same application .exe. Isn't there a way to make sure the memory layout reordering never occurs in visual studio? – Gam Jun 25 '17 at 14:19
  • @Phantom `#pragma pack`. – Hatted Rooster Jun 25 '17 at 14:22
  • _@Phantom_ @Gill OK, I did so myself recently. Not for communications between threads, but over the network. For the latter purpose I used `#pragma pack(1)` without using the `push` and `pop` around the `struct`. It costed a whole week and help of a colleague to find out what's causing the weird behavior of the application. Also POD could be easily broken by a future maintainer. So doing such stuff is certainly _not safe_. About the question if it's _legal_ I can't say from a standard cite, but as with every kind of `reinterpret_cast`'s I'd consider it is UB. – πάντα ῥεῖ Jun 25 '17 at 14:25
  • @πάνταῥεῖ I've used pragma pack(1) in the past, but you scared me with the "compiler can change memory layout". Ie, #pragma pack(1) struct {char a, char b }, I wasn't sure if the pragma pack would protect me from the struct becoming struct { char b, char a }. Also in my defense, this "hack" was gonna be used in a few places, where speed was heavily needed. I always used pragma pack with push and pop around the structs, otherwise the aligment would spread like cancer, specially if u have it in a header. I don't know why u didn't do the same, maybe that caused the bugs? – Gam Jun 25 '17 at 14:45
  • @Phantom Well, just don't forget `#pragma(push)` and `#pragma(pop)` as I did in [my case mentioned](http://i0.wp.com/hotnerdgirl.com/wp-content/uploads/2011/02/triple-facepalm.jpg?w=720). Your example will work, but be aware of UB. – πάντα ῥεῖ Jun 25 '17 at 14:48
  • @πάνταῥεῖ I actually ran the is_pod test since u made me doubt what I thought I knew, and it returns false, so you seem to be wrong (at least MSVC2017 says so). I also had edited my previous post before ur last one, so y I'm aware of the push/pop thing, thanks anyways. – Gam Jun 25 '17 at 15:01
  • @Phantom Can you prepare a minimal test case for this please? I'll check with my MSVC2017 installation as well. – πάντα ῥεῖ Jun 25 '17 at 15:20
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/147554/discussion-between---and-phantom). – πάντα ῥεῖ Jun 25 '17 at 15:33
-8

It is guaranteed to be successfully compiled without error messages. So by definition, it is 100% legal.

In this particular case, it would work as intended. But, if a structure uses virtual functions, it would store vptr, and this thing would not work. You may, for instance, later add a virtual function, and the constructor will stop working. So no, it is not safe.

user31264
  • 6,557
  • 3
  • 26
  • 40
  • 6
    '*So by definition, it is 100% legal.*' This may be syntactically true but it is semanctically false. "Compileable without error messages" neither implies standard conformance nor well defined runtime behaviour of a program. – Pixelchemist Jun 25 '17 at 14:30
  • 4
    Many kinds of _undefined behavior_ as specified in the standard won't result in compiler errors or warnings. So no. – πάντα ῥεῖ Jun 25 '17 at 14:30
  • @πάνταῥεῖ - undefined behaviour is not illegal, unless it may cause the program to crush. – user31264 Jun 25 '17 at 14:33
  • @πάνταῥεῖ - I wrote it is *unsafe*, but not *illegal*. Can you understand the difference? – user31264 Jun 25 '17 at 14:35
  • @user31264 Note that I also added that "I'm not sure if the C++ standard prohibits it", so the diff u r trying to make between illegal/unsafe is irrelevant. I only trust the standard, not the compiler. – Gam Jun 25 '17 at 14:41
  • @Phantom - how the first part of the sentence connected to the second part? And no, it is not me who makes the difference, but the original poster. – user31264 Jun 25 '17 at 14:42