Ensure key/value of enumerable type never changes between software revs

Question

We have a client/server application where older servers are supported by newer clients. Both support a shared set of icons.

We're representing the icons as an implicit enum that's included in the server and client builds:

enum icons_t { // rev 1.0
  ICON_A, // 0
  ICON_B, // 1
  ICON_C  // 2
};

Sometimes we retire icons (weren't being used, or used internally and weren't listed in our API), which led to the following code being committed:

enum icons_t { // rev 2.0
  ICON_B, // 0
  ICON_C  // 1 (now if a rev 1.0 server uses ICON_B, it will get ICON_C instead)
};

I've changed our enum to the following to try and work around this:

// Big scary header about commenting out old icons
enum icons_t { // rev 2.1
  // Removed: ICON_A = 0,
  ICON_B = 1,
  ICON_C = 2
};

Now my worry is a bad merge when multiple people add new icons:

// Big scary header about commenting out old icons
enum icons_t { // rev 30
  // Removed: ICON_A = 0,
  ICON_B = 1,
  ICON_C = 2,
  ICON_D = 3,
  ICON_E = 3 // Bad merge leaves 2 icons with same value
};

Since it's an enum we don't really have a way to assert if the values aren't unique.

Is there a better data structure to manage this data, or a design change that wouldn't be open to mistakes like this? My thoughts have been going towards a tool to analyze pull requests and block merges if this issue is detected.

Should be possible to write a [clang tidy](http://clang.llvm.org/extra/clang-tidy/) check to ensure values are unique.. — Jesper Juhl, Apr 04 '18 at 18:02
@JesperJuhl That sounds like it would work, but we're using `arm-none-eabi-g++` — codehearts, Apr 04 '18 at 18:04
Keep the old enum member and rename it to `ICON_A_DEPRECATED`. Then only add new ones at the end. — Cameron, Apr 04 '18 at 18:05
Since you mention the client and server and say that the `icons_t` must remain constant, I assume that the enums are converted to a number transfered over the network and then those numbers are converted back to enums, is that correct? — t.niese, Apr 04 '18 at 18:07
@t.niese Yes, the enums are converted to ints for network transmission and then converted back to enums when received — codehearts, Apr 04 '18 at 18:09
Then add a unit tests that check if the decoded enum corresponds to the previously decoded one. And if you do this test for every enum, then it should not be possible to either decode `ICON_D` or `ICON_E`, and fail for one of those. — t.niese, Apr 04 '18 at 18:15
I presume you would select a behavior based on a received `enum` value at some point. Perhaps using an `std::map` or a `switch`. You can make sure at that point that the enum is always unique. For example, `switch` won't allow equivalent `case` labels. If you use a `std::map` make sure that all insertions succeed and that a matching element wasn't already present. It won' — François Andrieux, Apr 04 '18 at 18:18
@t.niese We don't have unit tests for our enum values, doing so means we have the problem of ensuring tests are written and maintained now — codehearts, Apr 04 '18 at 18:30
If this is C++11 question, why not use `enum class` ? I mean you can avoid a lot of problems, by just using `enum class`. — badola, Apr 04 '18 at 18:51

score 1 · Accepted Answer · answered Apr 04 '18 at 18:23

I have previously done tests that check out previous builds and scan header files for this type of version-breaking behaviour. You can use diff to generate a report of any changes, grep that for the common pattern, and identify the difference between deleting a fixed-index entry, changing the index of an entry, and deleting or inserting a floating index entry.

The one obvious way to avoid it is to NOT remove the dead indices, but rename them, i.e. ICON_A becomes ICON_A_RETIRED, and its slot is reserved for ever. It is inevitable that someone will change an index, though, so a good unit test would also help. Forcing a boilerplate style means the test is simpler than coping with the generic case.

Another trick might be to accept that the issue will occur, but if it is only a problem for customers, and at each software release/revision, update a base number for the range, release the software and update again, so the dev version is never compatible with the release, eg

#define ICON_RANGE 0x1000
#define ICON_REVISION_BASE ((RELEASENUM+ISDEVFLAG)*ICON_RANGE)
enum icon_t {
  iconTMax = ICON_REVISION_BASE+ICON_RANGE,
  iconTBase = ICON_REVISION_BASE,
  icon_A,
  icon_B,

Then, at run-time, any icons not in the current range are easily rejected, or you might provide a special look-up between versions, perhaps generated by trawling your version control revisions. Note that you can only provide backward compatibility this way, not forward compatibility. It would be up to newer code to preemptively back-translate their icon numbers to send to older modules, which may be more effort than it is worth.

score 0 · Answer 2 · answered Apr 04 '18 at 18:45

This thought just crossed my mind: if we keep a literal at the end for the enum size, our unit tests can use that to assert if we haven't verified each enum literal:

enum icons_t {
  ICON_A_DEPRECATED,
  ICON_B,
  ICON_C,
  ICON_COUNT // ALWAYS KEEP THIS LAST
};

Then in testing:

unsigned int verifyCount = 0;
verify(0, ICON_A_DEPRECATED); // verifyCount++, assert 0 was not verified before
verify(1, ICON_B); // verifyCount++, assert 1 was never verified before
assert(ICON_COUNT == verifyCount, "Not all icons verified");

Then our only problem is ensuring tests pass before releasing, which we should be doing anyway.

You could add a static_assert to do this job at compile time. I'm not sure what it gives you when someone adds a new icon. Certainly having a LAST is a good idea. — Gem Taylor, Apr 05 '18 at 09:47

score 0 · Answer 3 · answered Apr 04 '18 at 19:23

Since the question has been tagged C++11, this could be better handled with Scoped enumerations.
Read about it here : http://en.cppreference.com/w/cpp/language/enum

Since the same enum file is included in both client and server, then removing any entry would lead to compilation failure in places a missing entry is being used.

All that need to be changed, is your icon_t.
Upgrade it from enum to enum class

enum class icon_t
{
    ICON_A,
    ICON_B,
};

Now you can't blatantly pass int instead of an icon_t. This reduces your probability to make mistakes drastically.

So the calling side

#include <iostream>

enum class icon_t
{
    ICON_A,
    ICON_B,
};

void test_icon(icon_t const & icon)
{
    if (icon == icon_t::ICON_A)
        std::cout << "icon_t::ICON_A";
    if (icon == icon_t::ICON_B)
        std::cout << "icon_t::ICON_B";
}

int main()
{
    auto icon = icon_t::ICON_A;
    test_icon(icon); // this is ok
    test_icon(1); // Fails at compile time : no known conversion from 'int' to 'const icon_t' for 1st argument 
    return 0;
}

Moreover, extracting numerical values is allowed from Scoped Enumerators. static_cast to int is allowed. If required.

int n = static_cast<int>(icon); // Would return 0, the index of icon_t::ICON_A

I've thought about this, but we must convert to int when transmitting over the network. If server 1.0 has `enum class {A, B, C}` and client 2.0 has `enum class {B, C}`, then the server would cast A to 0 and the client would cast 0 to B — codehearts, Apr 04 '18 at 19:33
You can't freely assign integers to unscoped enumerations either in c++, so I'm not sure what this gives you. Note that one of the latest standards does allow you to assign {1234} to a scoped enum without an explicit cast, which may be useful somewhere. — Gem Taylor, Apr 05 '18 at 10:01

Ensure key/value of enumerable type never changes between software revs

3 Answers3