tags:

views:

281

answers:

9

Let's say I have a constructor like this:

MyColor(uint8 vec[]) {
r = vec[0];
g = vec[1];
b = vec[2];
a = vec[3];
}

But I call it like this (3 elements instead of 4):

uint8 tmp[] = {1,2,3};
MyColor c(tmp);

But now vec[3] is undefined... is it safe to assign this value to a? If not, there's no nice workaround to check if vec[3] is set is there?

+18  A: 

No it's not safe. It's undefined behavior as defined by the standard. It might blow the whole app up or return a random value. The workaround is to pass the size along with it, or use vector instead.

Mehrdad Afshari
"undefined as defined by the standard"? ;)
jalf
A: 

Nope. It's possible this causes a segfault.

Billy3

Billy ONeal
+1  A: 

The shown behavior is undefined, and likely to crash down the road.

I would recommend to use vector along, and access elements using vector::at() call. That will throw an exception if went beyond alloted bounds. Ensure to reserve enough space in the vector

vehomzzz
+14  A: 

No, it's not safe. You're reading memory that you have not allocated, and this is undefined behavior. Depending on the phase of the moon you might or might not get a segmentation fault.

To "work around" it, make sure the array you pass in has always the correct size. In your example you could do:

uint8 tmp[4] = {1,2,3};
MyColor c(tmp);

The initializer doesn't need to specify all the elements of the array that gets created, so it's ok to just initialize the first three values explicitly. The remaining values will be set to zero.

sth
+5  A: 

In regards to the subject line: It is safe to save a dangling (or otherwise invalid) pointer. It's just not safe to dereference it, which your code does. Mostly a meaningless distinction, I know, since there's rarely a point in storing a pointer you're not going to dereference, but I thought I'd point this out for accuracy's sake.

sepp2k
+5  A: 

No, this is not safe. Maybe you would like to pass the coordinates as parameters to constructor and use two overloads:

MyColor(uint8 a, uint8 b, uint8 c, uint8 d) {
    stuff
}

MyColor(uint8 a, uint8 b, uint8 c) {
    stuff
}

This way you can use both

MyColor a(1, 2, 3);
MyColor b(1, 2, 3, 4);
Roman Plášil
I've already got overloads for those, but thanks :)
Mark
A: 

It's like you'r asking if it's safe to build a house on top of a lake. One day you may wake up in the water.

In your case you may find that a dangling pointer can cause unstability and frequent crashes to the system.

If you know what a dangling pointer is you now what the risk is.

Secko
Well, if the user doesn't specify the alpha value, it's assumed he won't try using it either. But I guess the mere assignment can cause destruction.
Mark
+3  A: 

if you dont want to use vector try this...

MyColor(uint8 (&vec)[3])
{
   r = vec[0];
   g = vec[1];
   b = vec[2];
}

MyColor(uint8 (&vec)[4])
{
   //...
}

uint8 a1[] = {1,2,3};
MyColor c1(a1);
uint8 a2[] = {1,2,3,4};
MyColor c2(a2);
uint8 a3[] = {1,2,3,4,5};
MyColor c3(a3); // error

you dont have to include the array's size explicitly and if you try to pass an array with wrong number of elements, compile error will be generated,

newbie
Now that's cool! Didn't know you could do that. Was trying to do that, but my syntax was wrong I guess.
Mark
+2  A: 

In C++, as in C, on function arguments arrays decay into pointers. Now, depending on the semantics of the function you can use other approaches:

// explicit parameters (with or without default values):
mycolor( uint8_t r, uint8_t g, uint8_t b, uint8_t alpha = 255 );

// vector (can check length)
mycolor( std::vector<uint8_t> const & components );

// array by reference
mycolor( uint8_t (&components)[ 4 ] );

The first approach to me is the cleanest.

David Rodríguez - dribeas
Defining a struct/class Colour (or whatever) which has the four elements is probably even better..
Kristof Provost