current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

If you were given the following:

#include <cassert>

struct vec {
    float x,y,z;
    vec(float x, float y, float z) : x(x), y(y), z(z) {}
};

int main() {
    float verts[] = {1,2,3,4,5,6,7,8,9}; // length guaranteed to be a multiple of 3
    assert(sizeof(vec) == sizeof(float) * 3);

    // Option 1
    for (int i = 0; i < 3; ++i) {
        auto a = reinterpret_cast<vec*>(verts)[i];
        // ...
    }

    // Option 2
    for (int i = 0; i < 9; i += 3) {
        vec a(verts[i], verts[i+1], verts[i+2]); // -O3 averts the copy, so d/w
        // ...
    }

    return 0;
}

Which is considered better practice?

The advantage to the latter option is of course far more explicit, and carries all the advantages/disadvantages that contains; but may not be portable depending on the assertions you can make.

share

locked by Jamal♦ Apr 1 '14 at 2:46

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.
up vote 3 down vote accepted

I would go with a 3rd option that is closest to option 2. But instead of taking the three parameters separately in the constructor, take an array of floats.

struct vec {
    float x,y,z;
    vec(float fv[]) : x(fv[0]), y(fv[1]), z(fv[2]) {}
};

int main() {
    float verts[] = {1,2,3,4,5,6,7,8,9};

    for (int i = 0; i < 9; i += 3) {
        const vec a(&verts[i]);
        // ...
    }

    return 0;
}
share
    
A great idea, averts extraneous syntax, clear intent, and all the behaviour is defined. Also allows for the original data to be stored as floats. – dcousens Aug 25 '11 at 23:09
up vote 1 down vote

I strongly prefer option 3.

Options 1 and 2 remind me most of K&R C where a pointer is a pointer is a pointer. If one of the types is changed, the compiler may not notice and that can lead to bugs that may be hard to locate.

Option 3 is the style used in a strongly typed language. If one of the types is changed, the compiler is more likely to notice and complain at the point where code needs to be fixed.

share
up vote 1 down vote

Why not do this:

struct vec
{
    float x,y,z;
};


int main()
{
    vec verts[] = {{1,2,3}, {4,5,6}, {7,8,9}};

It is definitely more clear than any of the proposed solutions.
Is type safe (a key point) and avoids the copy you want (as they are created in place).

// Option 4
for (int i = 0; i < 3; ++i)
{
    const vec& a    = verts[i];

}

When writing C++ code you should definitely not use the C cast operator. C++ has four of its own that you can use:

const_cast           // If you need this you are either very clever or your design is wrong
reinterpret_cast     // This is an indication that you are doing something non portable.
static_cast          // This is a good cast (but any explicit cast is bad)
dynamic_cast         // This casts up and down the class hierarchy (usually just down)

                     // Note: dynamic_cast is a runtime operation.
                     //       all others are compile time casts.
                     //       static_cast/dynamic_cast will actually generate compile
                     //       time errors if you do something wrong very wrong. dynamic_cast
                     //       can also throw an exception (or return NULL).

In your case when I convert your C-casts into C++

   // option 1
   const vec& a = reinterpret_cast<vec&>(verts[i]);

   // Option 2
   const vec& a = reinterpret_cast<vec*>(verts)[i];

Now if you present your code to a C++ developer the first thing they will do is ask: why do you need reinterpret_cast? That's a bad idea.

share
    
A valid point, but some libraries in my code require the data as a pointer to floats. The POD way triumphs unless I use reinterpret cast for these situations (agreed, yuk). – dcousens Aug 25 '11 at 16:49
up vote 0 down vote

I'd say, use the last one until this prooves to by a performance problem (which I highly doubt). Also, be aware that the first two solution would modify the array, should the following code modify a.

share
    
The compile generally brings this all down to the same code (or close enough, perhaps 2 different ops), I'm just asking from a programmers point of view; and perhaps from a standards point of view as well. – dcousens Aug 25 '11 at 15:50

Not the answer you're looking for? Browse other questions tagged or ask your own question.