views:

1081

answers:

7

I wrote a small coordinate class to handle both int and float coordinates.

template <class T>
class vector2
{
public:
    vector2() { memset(this, 0, sizeof(this)); }
    T x;
    T y;
};

Then in main() I do:

vector2<int> v;

But according to my MSVC debugger, only the x value is set to 0, the y value is untouched. Ive never used sizeof() in a template class before, could that be whats causing the trouble?

+4  A: 

The problem is this is a Pointer type, which is 4 bytes (on 32bit systems), and ints are 4 bytes (on 32bit systems). Try:

sizeof(*this)

Edit: Though I agree with others that initializer lists in the constructor are probably the correct solution here.

jeffamaphone
That worked! :) But I cant remember doing that dereference before, just 'this'. Do my memory fail me or is this usually the way its done? Im digging through my code right now to find other places where Ive done something similar.
mizipzor
This is the way sizeof() has always worked... though really you should consider the initializer list, assuming 0 is always compatible with the template types you plan to use.
jeffamaphone
Downvoted -- memset should never be used for initialization in C++, with the *possible* exception of POD structs. See other answers regarding memset and virtuals.
Dan Breslau
Which is why I added the edit...
jeffamaphone
You can also delete the answer. Or, at least, remove the "right" way to use memset, since there is no right way in this case.
Dan Breslau
+22  A: 

No don't use memset -- it zeroes out the size of a pointer (4 bytes on my x86 Intel machine) bytes starting at the location pointed by this. This is a bad habit: you will also zero out virtual pointers and pointers to virtual bases when using memset with a complex class. Instead do:

template <class T>
class vector2
{
public:
    // use initializer lists
    vector2() : x(0), y(0) {}
    T x;
    T y;
};
dirkgently
Ive heard that although structs arent classes, you can use that "constructor" to zero out all members. And a memset catches everyone even if you add some more later. Isnt there a way to make memset work?
mizipzor
In C++ a struct has the exact same ability to hold member functions as any classes. Just some differeneces like: the members have a default public visibility etc. For a POD, as is vector2, this works. But it is always better to stick to one style.
dirkgently
+1, good point about virtual table pointers and virtual bases
Adam Rosenfield
mizipzor, even if there was "set all numeric members to zero" syntax in C++, it would be useless. In all but the simplest cases: (A) you don't want to init all numerics to 0, (B) you want to initialize members of class type meaningfully, and (C) adding more members doesnt mean you want them zeroed.
+2  A: 

Don't try to be smarter than the compiler. Use the initializer lists as intended by the language. The compiler knows how to efficiently initialize basic types.

If you would try your memset hack on a class with virtual functions you would most likely overwrite the vtable ending up in a disaster. Don't use hack like that, they are a maintenance nightmare.

lothar
@lothar: He is probably reading Lipman's C++ Primer ;)
dirkgently
Actually no, its all me. So dont read my book if I write one, hehe. ;)
mizipzor
+12  A: 

As others are saying, memset() is not the right way to do this. There are some subtleties, however, about why not.

First, your attempt to use memset() is only clearing sizeof(void *) bytes. For your sample case, that apparently is coincidentally the bytes occupied by the x member. The simple fix would be to write memset(this,0,sizeof(*this)), which in this case would set both x and y.

However, if your vector2 class has any virtual methods and the usual mechanism is used to represent them by your compiler, then that memset will destroy the vtable and break the instance by setting the vtable pointer to NULL. Which is bad.

Another problem is that if the type T requires some constructor action more complex than just settings its bits to 0, then the constructors for the members are not called, but their effect is ruined by overwriting the content of the members with memset().

The only correct action is to write your default constructor as

vector2(): x(0), y(0), {}

and to just forget about trying to use memset() for this at all.

Edit: D.Shawley pointed out in a comment that the default constructors for x and y were actually called before the memset() in the original code as presented. While technically true, calling memset() overwrites the members which is at best really, really bad form, and at worst invokes the demons of Undefined Behavior.

As written, the vector2 class is POD, as long as the type T is also plain old data as would be the case if T were int or float.

However, all it would take is for T to be some sort of bignum value class to cause problems that could be really hard to diagnose. If you were lucky, they would manifest early through access violations from dereferencing the NULL pointers created by memeset(). But Lady Luck is a fickle mistress, and the more likely outcome is that some memory is leaked and the application gets "shaky". Or more likely, "shakier".

The OP asked in a comment on another answer "...Isn't there a way to make memset work?"

The answer there is simply, "No."

Having chosen the C++ language, and chosen to take full advantage of templates, you have to pay for those advantages by using the language correctly. It simply isn't correct to bypass the constructor in the general case. While there are circumstances under which it is legal, safe, and sensible to call memset() in a C++ program, this just isn't one of them.

RBerteig
Actually the default constructors for x and y are both called in the OP. Not that this makes using memset any less atrocious. In your case, the conversion constructor from int -> T will be called instead.
D.Shawley
True. And then the memset cruelly slammed the bits back to all zero, after the complicated type T finished its default construction.
RBerteig
+4  A: 

Don't use memset. It'll break horribly on non-POD types (and won't necessarily be easy to debug), and in this case, it's likely to be much slower than simply initializing both members to zero (two assignments versus a function call).

Moreover, you do not usually want to zero out all members of a class. You want to zero out the ones for which zero is a meaningful default value. And you should get into the habit of initializing your members to a meaningful value in any case. Blanket zeroing everything and pretending the problem doesn't exist just guarantees a lot of headaches later. If you add a member to a class, decide whether that member should be initialized, and how.

If and when you do want memset-like functionality, at least use std::fill, which is compatible with non-POD types.

If you're programming in C++, use the tools C++ makes available. Otherwise, call it C.

jalf
memset() is normally well optimized and I'd expect it to be faster if assignements aren't translated to a call to memset() by the compiler.
Bastien Léonard
memset is well optimized for what it's designed to do, which is set a range of memory to one specific value. Using it to set two ints is likely slower.
jalf
but like I said, if memset-like functionality is needed, use std::fill. Since that's defined in headers only, it can be trivially inlined. Memset requires a lot more compiler trickery to inline.
jalf
@Bastien Léonard: If memset is the fastest way to implement the initializers for adjacent fields, then even a trivially optimizing compiler will use memset (or its equivalent.) As lothar said, don't try to be smarter than the compiler.
Dan Breslau
+2  A: 

dirkgently is correct. However rather that constructing x and y with 0, an explicit call to the default constructor will set intrinsic types to 0 and allow the template to be used for structs and classes with a default constructor.

template <class T>
class vector2
{
public:
    // use initializer lists
    vector2() : x(), y() {}
    T x;
    T y;
};
Stephen Nutt
+1  A: 
Martin