3

I'm replacing...about 6000 lines of deserialization C code with what I hope is about 3000 lines of C++. The basic existing pattern is like so:

ser_ret_t msgpack_unpack_mystruct(msgpack_object *obj, mystruct_t *d, void *unused = NULL)
{
  NEXT_WORK_OBJ(array, i, work_obj);
  ret |= msgpack_unpack_line(work_obj, &d->line);
  NEXT_WORK_OBJ(array, i, work_obj);
  ret |= msgpack_unpack_node(work_obj, &d->leftNode);
  //etc....

This is repeated once for each variable within a given struct. array, i, and work_obj are all state variables used by msgpack, and d is an arbitrary structure pointer. NEXT_WORK_OBJ is a preprocessor macro that is needed to adjust the state variables. msgpack_unpack_* is a function with arbitrary parameters.

This is not only verbose, but incomplete, as the underlying buffer continues to be processed even in the case of a failure.

My thought is to replace the state variables with a state/helper class, with the one issue being calling an arbitrary function within the class. I'm handling that by not making the call within the class, but via lambda.

class UnpackHelper
{
public:
  void GetNext(std::function<ser_ret_t(msgpack_object*)> callback)
  {
    //make sure it's safe to continue processing buffer.
    if (m_status != SER_SUCCESS)
      return;
    NextObj();
    m_status |= callback(m_pWork);
  }
};

NextObj() replaces the preprocessor macro, and adjusts the class internal state variables. Now the client code looks like so:

ser_ret_t msgpack_unpack_mystruct(msgpack_object *obj, mystruct_t *d, void *unused = NULL)
{
  UnpackHelper up;
  up.GetNext([=](auto pWork) {return msgpack_unpack_line(pWork, &d->line); });
  up.GetNext([=](auto pWork) {return msgpack_unpack_node(pWork, &d->leftNode);});

My question is specifically about the client code, and the lambdas. My understanding is that a default capture is happening here, and I further understand that those are not recommended. Because the capture is always going to happen on a member of a pointer passed into the function (d), though, I believe this pattern is safe. Is this correct? Given the thousands of lambdas that will exist, I would like to avoid unnecessary verbosity. Second, is it possible to shorten or remove the parameter list in any way? (auto pWork) is fairly minimal, but...

My new, third question, based on learning about std::function..is my callback mechanism even necessary? std::function looks like something I might be able to use to create an arbitrary call directly within the class. I'm currently looking at the accepted answer here: C++: pass function with arbitrary number of parameters as a parameter to see if it can be applied to my code...without using C-style variadic args. Looks like this could work, but buys me nothing but code bloat caused by template expansion.

zzxyz
  • 2,953
  • 1
  • 16
  • 31
  • There seems to be no capture at all here. – François Andrieux Aug 27 '18 at 18:47
  • `&d->line` and `&d->leftnode` in the last codeblock are not captures? – zzxyz Aug 27 '18 at 18:48
  • 1
    The code you post fails to compile as you don't capture `d`. Can you fix that typo? `[]` means cappture nothing. And once you capture something, lambda does not convert to function pointer... – Yakk - Adam Nevraumont Aug 27 '18 at 18:49
  • 1
    This code will not compile. You need `[=]` if you want to implicitly capture `d`. Doing that though will not allow you to use a function pointer and you will have to switch to using a `std::function` instead. – NathanOliver Aug 27 '18 at 18:49
  • I think you mean default capture by value or reference [like here](https://stackoverflow.com/q/34568183/2754173). The `[]` syntax is the *empty capture* – Geezer Aug 27 '18 at 18:51
  • Okay. Sincere apologies. I will fix the various `WIP` issues my code has, and present something that will compile. I was relying on Intellisense to highlight compile issues, which is hardly bulletproof. – zzxyz Aug 27 '18 at 18:52
  • Okay, the code displayed is obviously still incomplete, but does compile now within its full framework. @NathanOliver - thanks for that tip. I woulda been stumped on that function pointer for a bit. – zzxyz Aug 27 '18 at 19:09
  • You know if you use a stream inspired interface this might be a lot less messy to implement. I hope you have a lot of unit testing code in any case to verify that this new implementation works as well as the old one does. – tadman Aug 27 '18 at 21:53
  • @tadman - Yeah, this msgpack implementation is not how I would've done things. There's definitely unit testing code, but I find it's an *extremely* poor guard against undefined behavior. Compounding problems is that most of this stuff is being unpacked into a custom heap. Anyway, if I can get rid of these particular preprocessor macros, and shrink the code size, it's a win. – zzxyz Aug 27 '18 at 22:48
  • Concerning C++ templates, VS2013 doesn't highlight anything. It seems that Intellisense cannot analyze template code. (I wouldn't wonder.) In general, don't trust Intellisense too much. It's a fine/impressive tool but it cannot do as exactly as the compiler does. – Scheff's Cat Aug 28 '18 at 05:53
  • Unit tests are great at verifying that the code functions as intended within a range of inputs, both good and bad. To expose undefined behaviour you need to "fuzz" it by deliberately damaging input in random ways to trip it up or hit up your code with tools like [Valgrind](http://valgrind.org) that expose deeper, more subtle flaws. – tadman Aug 28 '18 at 13:55
  • @tadman valgrind doesn't tend to expose a lot of flaws it otherwise might in this code what with the custom heap. I think the title of my autobiography will be "F you, custom heap writers" subtitled "Why are you all incompetent?" – zzxyz Aug 28 '18 at 21:05
  • 1
    Part of the "So you want to write a custom heap, now what?" series? Not sure if there's something like [Flay](https://github.com/seattlerb/flay) for C++ but it shakes out a lot of holes in your test coverage pretty fast. – tadman Aug 28 '18 at 21:08
  • `1 file changed, 1610 insertions(+), 4788 deletions(-)` Soooo much more readable now, too. – zzxyz Aug 30 '18 at 20:30

0 Answers0