search

intel / tinycbor

Concise Binary Object Representation (CBOR) Library
MIT License
489 stars 187 forks source link

Document and improve abstract reader/writer interface #208

Open sjlongland opened 3 years ago

sjlongland commented 3 years ago

tinycbor fork branch thiagomacieira/dev adds support for abstract readers and writers, but the implementation had some limitations in cases where multiple CborValue cursors were iterating over the same CBOR document, causing state contamination. The interface also was not documented.

This documents the new interface and further builds upon it by slightly re-arranging CborParser members and opening the token in CborValue to use by the reader interface for any required purpose.

This pull request is now re-based on intel/dev and supercedes https://github.com/thiagomacieira/tinycbor/pull/2.

sjlongland commented 3 years ago

Forced commits to:

  1. remove stale commit from the old thiagomacieira/dev
  2. fix authorship of commits so that Github's PGP signature verification is happy.
sjlongland commented 3 years ago

Thank you for your patience. This is looking really good. I will not insist on coding style changes in the examples (they're your code) nor on the auto usage. The reinterpret_cast is a different case, please update that and please move the Doxygen docs to the .c sources (See https://www.doxygen.nl/manual/docblocks.html#structuralcommands).

Ahh, the auto and reinterpret_cast are copied and pasted from the existing test cases. I normally avoid such things but was trying to match the style of the existing test cases. I will fix that. :-)

Finally, what's WSHUB-458? Sounds like a JIRA task number. Can you add a link to the commit message body to what this is and remove from the first line?

Good point, force of habit due to the fact I was doing this at work. I'll strip that token from the commit messages.

thiagomacieira commented 3 years ago

(oops, accidentally edited instead of quoting)

Ahh, the auto and reinterpret_cast are copied and pasted from the existing test cases. I normally avoid such things but was trying to match the style of the existing test cases. I will fix that. :-)

I'm using the Qt rules for auto: it has to be clear what it is, for someone reviewing on a dumb tool like GitHub. That usually means obvious (like iterators) or the type is visible on the same line.

sjlongland commented 3 years ago

I'm using the Qt rules for auto: it has to be clear what it is, for someone reviewing on a dumb tool like GitHub. That usually means obvious (like iterators) or the type is visible on the same line.

Agreed, code clarity must be paramount. I do more C than C++ so not used to using C++ features like auto and tend to avoid using it for that reason.

thiagomacieira commented 3 years ago

Gah, Travis hasn't run yet... Need to find someone to write GitHub Actions workflow, though likely that'll be me.

sjlongland commented 3 years ago

Ahh travis.org is no more… I hit this on the weekend with one of my own projects (aioax25) and it's a landmine waiting to go blam for some of the @vrtsystems and @widesky projects (pyat, cachefs, hszinc) that are still set up to do CI on Travis-CI. (And sadly, Github Actions doesn't hold a candle to Travis-CI, but Travis-CI is expensive now.)

thiagomacieira commented 3 years ago

Ahh travis.org is no more… I hit this on the weekend with one of my own projects (aioax25) and it's a landmine waiting to go blam for some of the @vrtsystems and @widesky projects (pyat, cachefs, hszinc) that are still set up to do CI on Travis-CI. (And sadly, Github Actions doesn't hold a candle to Travis-CI, but Travis-CI is expensive now.)

Too bad. Thanks for the update. Let me do a manual check locally and then force the merging.

sjlongland commented 2 months ago

So, last time we looked at this, we got caught out by Travis CI's shutdown. I've just done a rebase on the current dev branch to poke CI again.

sjlongland commented 2 months ago

Second rebase to pick up the changes on main that weren't merged to dev.

sjlongland commented 2 months ago

Okay, after picking up the changes in main, we have a merge conflict, so final rebase onto dev again to resolve the merge conflict. This should unify the currently diverging main and dev branches once more.