Adding a field to a structure without breaking existing code

Question

So I'm working with this huge repository of code and have realized that one of the structs lack an important field. I looked at the code (which uses the struct) as closely as I could and concluded that adding an extra field isn't going to break it.

Any ideas on where I could've screwed up?

Also: design advice is welcome - what's the best way I can accomplish this?

E.g. (if I wasn't clear):

typedef struct foo
{
  int a;
  int b;
}
foo;

Now it's :

typedef struct foo
{
  int a;
  int b;
  int c;
}
foo;

Answer 1

If you are using sizeof(struct) to allocate memory at all places and are accessing the members using -> or . operators, I don't think you should face any problem. But, it also depends on where you are trying to add the member, it might screw up your structure alignment if you are not careful.

Answer 2

Any ideas on where I could've screwed up?

Nothing. Everything. It all depends on how, where and why this is used.

Assuming this structure you talk about is a C-style POD and the code is but the simplest, you'll get away with it. But, the moment you are trying something more ambitious, you are dealing with alignment issues (depending on how and where you create objects) and padding at least. If this is C++ and your POD contains custom operators/ctors etc -- you're getting into a lot of trouble. Cross-platform issues may arise, if you rely on the endianness ever etc.

Answer 3

From what you've written above I can't see anything wrong. Two things I can think of:

1. Whenever you change code and recompile you introduce the ability to find "hidden" bugs. That is, uninitialized pointers which your new data structure could be just big enough to be corrupted.
2. Are you making sure you initialize c before it gets used?

Follow Up:

Since you haven't found the error yet I'd stop looking at your struct. Someone once wrote look for horses first, zebras second. That is, the error is probably not an exotic one. How much coverage do you have in your unit tests? I'm assuming this is legacy code which almost invariably means 0% or at least that's been my experience. Is this accurate?

Answer 4

If that structure is being serialized/deserialized anywhere, be sure to pay attention to that section of the code.

Double check areas of the code where memory is being allocated.

Answer 5

If the code is used to transfer data across the network, you could be breaking things.

Answer 6

If adding a structure member anywhere other than as the first member breaks anything, then the code has undefined behaviour and it's wrong. So at least you have someone else (or your earlier self) to blame for the breakage. But yes, undefined behaviour includes "happens to do what we'd like it to do", so as the other guys say, watch out for memory allocation, serialization (network and file IO).

As an aside, I always cringe when I see typedef FOO ... struct FOO, as if one is trying to make C code look like C++. I realize I'm in a minority here :)

Answer 7

Look for memcpy, memset, memcmp. These functions are not member-wise. If they were used using the previous structure length, you may have problems.

Also search the files for every instance of the struct. There may be functions or methods that do not use the new important field. As others have said, if you find the structure in a #define or typedef, you'll have to search those too.

Answer 8

Since you tagged your question C++:

For the future, Pimpl/d-Pointer is a strategy that allows you much greater freedom in extending or re-designing your classes without breaking compatibility.

For example, if you had originally written

// foo.h
class Foo {
public:
    Foo();
    Foo(const Foo &);
    ~Foo();
    int a() const;
    void a(int);
    int b() const;
    void b(int);
private:
    class FooPrivate *const d;
};

// foo.c
class FooPrivate {
public:
    FooPrivate() : a(0), b(0) {}
    FooPrivate(const FooPrivate &o) : a(o.a), b(o.b) {}
    int a;
    int b;
};
Foo::Foo() : d(new FooPrivate()) {}
Foo::Foo(const Foo &o) : d(new FooPrivate(*o->d)) {}
Foo::~Foo() { delete d; }
int Foo::a() const { return d->a; }
void Foo::a(int a) { d->a = a; }
// ...

you can easily extend this to

// foo.h
class Foo {
public:
    // ...
    int a() const;
    void a(int);
    int b() const;
    void b(int);
    int c() const;
    void c(int);
    // ...
};

// foo.c
class FooPrivate {
    // ...
    int a;
    int b;
    int c;
};
// ...

without breaking any existing (compiled!) code using Foo.

Answer 9

If the code had a robust set of unit tests, it would probably be much easier to track down the problem (you asked for design advice ;) )

I assume you don't need to use the new 'c' variable everywhere in this giant codebase, you're just adding it so you can use it in some code you're adding or modifying? Instead of adding c to foo, you could make a new struct, bar, which contains a foo object and c. Then use bar where it's needed.

As for the actual bug, it could be anything with so little information to go on, but if I had to guess, I'd say someone used a magic number instead of sizeof() somewhere.

Answer 10

Its always safe to add new elements at the end of a C struct. Event if that struct is passed to different processes. The code which has been recompiled will see the new struct member and the code which hasn't been will just be aware of the old struct size and just read the old members its knows about. The caveat here is that new member has to be added at the end of the structure and not in the middle.