0

This question is based on this separate question on stack overflow.

I have a very low-level structure meant to compactly save presets on flash memory. For simplicity I am going to use stored_record as a substitute, obviously the actual type has a /lot/ more fields.

struct stored_record
{
    int16_t foo;
};

Now I also need to export/import this via the wire and I am using protobuf for this. So I need convert from the protobuf format to the compressed representation and back. I also have a loader submodule which should be able to load both the packed format and the protobuf one. For this reason I want to make a unified interface for both of them which is going to handle the conversions from the high-level values to each format. Let's define record_interface for this task:

class record_interface
{
public:
   virtual ~record_interface() = default;

   virtual void set_foo(BoundedFloat) = 0;
   virtual BoundedFloat get_foo() const = 0;
};

So far so good.

Now submodules may accept references to either mutable record_interfaces or const record_interfaces and act accordingly. This is a very simple and clean mental model.

Now we have two choices. One is that we write a record-like simple implementation of the above interface and we convert to it and back between the other representations. While simple this has the disadvantage that it's possible to forget to convert one of the fields. If we just have setters and getters, it's not easy to guarantee that we don't forget anything.

The other is to make our various representations implement the above interface. This forces us to implement conversions for all fields and we can even define a generic copy constructor by using it (eg by matching getters with setters).

One issue lies in that stored_record above must be trivially copyable so it can't implement the interface directly. One therefore needs to write a wrapper class:

class record_wrapper : public record_interface
{
public:
  DISALLOW_COPY_AND_ASSIGN(record_wrapper);

  record_wrapper(stored_record & wrapped)
    : wrapped_(wrapped) {}
  
  void set_foo(BoundedFloat value) final { wrapped_.foo = convert_to_int16(value); }
  BoundedFloat get_foo() const final { return convert_from_int16(wrapped_.foo); }

private:
  stored_record & wrapped_;
};

However this wrapper can only be used to wrap mutable references, it's not possible to use const references with the above. There does not seem to be a very clean solution to this problem. We can't store const references conditionally since we wouldn't be able to implement the non-const member functions of the interface. One idea touched in the original thread is to make a safe create_wrapper function that const_casts away const-ness but forces the resulting wrapper to be const:

template <class T>
class const_wrapper
{
public:
    template <class... Args>
    const_wrapper(Args&& ... args)
        : wrapped_(std::forward<Args>(args)...)
    {
    }

    T const & get() const { return wrapped_; }
    operator T const & () const { return wrapped_; }
    T const * operator->() const { return &wrapped_; }

private:
    T wrapped_;
};

record_wrapper make_wrapper(stored_record & wrapped) {return {wrapped}; }
const_wrapper<record_wrapper> make_wrapper(stored_record const & wrapped) { return {const_cast<stored_record &>(wrapped)}; }

This works properly and allows us to use the original interface without problems. But it feels like a bit of a hack.

The other option is to split the interfaces into a Reader/Writer pair (code is taken from here)

class const_record_interface
{
public:
  virtual ~const_record_interface() = default;
  virtual BoundedFloat get_foo() const = 0;
};

class mutable_record_interface : public const_record_interface
{
public:
  virtual void set_foo(BoundedFloat) = 0;
};

//~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

class const_record_wrapper : public const_record_interface
{
public:
  const_record_wrapper(const stored_record &wrapped) : wrapped_{wrapped} {}
  BoundedFloat get_foo() const final { return convert_from_int16(wrapped_.foo); }
private:
  const stored_record &wrapped_;
};

const_record_wrapper
make_wrapper(const stored_record &wrapped)
{
  return {wrapped};
}

class mutable_record_wrapper : public mutable_record_interface
{
public:
  mutable_record_wrapper(stored_record &wrapped) : wrapped_{wrapped} {}
  auto as_const() const { return make_wrapper(this->wrapped_); }
  void set_foo(BoundedFloat value) final { wrapped_.foo=convert_to_int16(value); }
  BoundedFloat get_foo() const final { return as_const().get_foo(); }
private:
  stored_record &wrapped_;
};

mutable_record_wrapper
make_wrapper(stored_record &wrapped)
{
  return {wrapped};
}

This is more sane but requires more boilerplate. It also makes const-ness on the reader/writers irrelevant as mutable vs const reference on the reader does not make a difference while const on the writer is useless.

I would really appreciate any feedback regarding this matter since the design space is very large and I am not sure which is the better way forward.

EDIT:

After thinking about it I came to realize the biggest reason that I am skeptical of the the split interface solution and that is that it forces us to redesign the whole interface hierarchy. Most user defined types could directly implement the original interface without problems. The interface's methods are also very coupled together and we wouldn't originally think of splitting them. The only reason the original interface does not work in our case is because we can't directly implement the interface on the stored_record type so we have to deal with contness of the wrapper. With the first solution we don't need to change our mental model just to fit with the edge case, with the second we have to redesign everything. So the first solution makes design easier but is not very idiomatic while the second one is very idiomatic but forces us to change our current design's model.

So in the end I think I would like to compare the two interfaces as standalone models and probably go with that. So it (most likely) boils down to:

User and writer ergonomics - Mutable/Const interfaces vs single interface with const and non-const methods.

1

I would prefer the second option. It is similar to iterator and const_iterator types provided by containers. Your wariness about constness of the wrapper is equally applicable to iterators and pointers.

You can have mutable_record_wrapper inherit const_record_wrapper, which removes some of the boilerplate.

class const_record_wrapper : public const_record_interface
{
public:
  const_record_wrapper(const stored_record &wrapped) : wrapped_{wrapped} {}
  BoundedFloat get_foo() const final { return convert_from_int16(wrapped_.foo); }
protected:
  const stored_record &wrapped_;
};

class mutable_record_wrapper : public mutable_record_interface, public const_record_wrapper
{
public:
  mutable_record_wrapper(stored_record &wrapped) : const_record_wrapper{wrapped} {}
  void set_foo(BoundedFloat value) final { const_cast<stored_record &>(wrapped_).foo=convert_to_int16(value); }
};

As an aside, you should probably prohibit rvalues from make_wrapper, to avoid dangling references. Lifetime extension does not propagate past the first reference.

const_record_wrapper make_wrapper(stored_record &&) = delete;
const_record_wrapper make_wrapper(const stored_record &&) = delete;
| improve this answer | |
  • Thanks a lot for the reply, it sure makes sense if you look at it that way. I added a small note at the end with the latest edit explaining my main problem with this approach. Maybe this is a non-issue after all though. – Liarokapis Alexandros Jun 25 at 11:30
1

From what I get, you are torn between 2 possible ways to implement the same idea.

  • Separated classes for const and mutable wrappers over the same interface.
  • Reader/Writer approach where you separate the ability to write on fields with different interface classes.

From a performance perspective I expect similar results so the choice is purely aesthetical. The first approach is simple while the second is more idiomatic, so let me outlay the benefits of each.

Wrapper Constness

Good

  • Easy to use, since you can differ if it is constant or not by looking at the wrapper and not the data.
  • Less code to implement.
  • Easier mental model (very important if you have young developers on your team).

Bad

  • Easy to misuse. If someone has a mutable wrapper at some point in the execution, there are no ways to pass an immutable wrapper unless you add such a function. This will be kind of a hack too.
  • Code duplication. The mutable interface has to implement the getters too.

Interface Constness

Good

  • Less code duplication.
  • Idiomatic.
  • Everything in the const classes remains constant all the way up the class hierarchy.
  • You can make a mutable interfaces into an immutable one and pass it around with possibly less cost, since you will just have to cast it the down.

Bad

  • Way more code to write.
  • Different and more complex mental model you will have to adapt to.

I will argue it will be worse to implement, since will have to write 2 interfaces and 2 wrappers instead if 1 and 2, possibly increasing binary size and also change your performance. I would suggest to do some performance testing on both cases.

I think both approaches do the job, so use the one easier to implement or the one you already have. If the one you choose ends up being a problem change it to the other, it probably won't be too difficult.

Personally I like the second approach more and if I had to choose I would choose the second one, although the first one is ok with me too!

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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