I'm not terribly involved anymore, but since I ended up writing most of that, I'll respond. I will say that it is based on the examples from the tinycbor library, I'm not sure how I would have done it otherwise.
The err = err | <cbor function> is only used in a few places where knowing the error afterwards is important, for most situations, the error code is pretty meaningless since there is nothing we can do about it. For the most part, it does err = err || ... Most of the errors are unrecoverable and meaningless, so checking it constantly isn't very useful, and would get pretty awful really quickly. Some more explanations in line. On Fri, 2015-10-23 at 15:30 +0000, Light, John J wrote: > I?m not sure where to direct this issue. > > > > Throughout the payload code in IoTivity there is a pattern like this: > > > > err = err | <cbor function> > > err = err | <another cbor function> > > ? > > > > The result is accumulation of error codes in ?err? and less error code > testing. > > > > I see two problems with this: > > ? If an earlier function fails to properly set a variable > (because of an error it reports) needed by a subsequent function, the > subsequent function will still execute, possibly leading to data > corruption or a segment violation. This is correct, however tiny-cbor guarantees that data corruption/segment violations won't happen except in a few specific instances. > > ? If a subsequent function also generates an error, but with a > different value, the value of ?err? will be combined in a way that the > resulting value of ?err? will incorrect or invalid. They aren't really checked in these cases, since most of the error conditions are 'write-time' errors or bad data. The response to 'write time errors' is to debug them, and the response to 'bad data' is to ignore it anyway, which can be done fine either way. > > > > While this code ?works? for the normal case, it is fragile in the face > of boundary conditions. This state seems inappropriate for IoTivity?s > long term. > > > > I am familiar with a similar pattern that looks like this: > > > > err = err || <function> > > err = err || <another function> > > ? > We actually use that in a significant amount of the code. > > > In this case, if any function returns a non-zero error code, > subsequent functions are not executed, but the original error code is > lost. This loss can be mitigated by this coding pattern: > > > > err = err || (errcode = <function>) > > err = err || (errcode = <another function>) > > ? > > > > This provides the error code, but it starts getting hard to scan. I don't mind that, but like I said, we don't really use the error code for much since it is pretty useless. > > > > But for our (IMHO, broken) coding conventions, we could consider this > pattern: > > > > err = <function> > > if (err) goto errorhandler; > > err = <another function> > > if (err) goto errorhandler; > > > > Which is both robust and readable (again, IMHO). > I think this is even more difficult to scan, is pretty arduous, and less readable than the previous versions. Making 50% of the lines in a function 'if (err)' seems pretty awful to me. > > > In any case, I believe the payload code should be reconsidered. > I definitely think we should be open to suggestions, though I don't think that the ones presented here are better than what is already there. > > > John Light > > Intel OTC OIC Development > > > > > > > _______________________________________________ > iotivity-dev mailing list > iotivity-dev at lists.iotivity.org > https://lists.iotivity.org/mailman/listinfo/iotivity-dev
