Open kkysen opened 2 years ago
Could we instead wrap a Rust cbor crate with C bindings and use that from c2rust-ast-exporter
? That way cargo
handles all the dependencies and we use the same codebase for encoding and decoding cbor. Speaking of that, serde_cbor
that we use is now archived and no longer maintained. Maybe we should switch to something like minicbor
, which serde_cbor
recommends and it also has a lot of downloads.
@milahu, let's discuss cbor stuff here.
We just use these functions:
cbor_encode_boolean
cbor_encode_byte_string
cbor_encode_double
cbor_encode_int
cbor_encode_null
cbor_encoder_close_container
cbor_encoder_create_array
cbor_encoder_get_buffer_size
cbor_encoder_get_extra_bytes_needed
cbor_encoder_init
cbor_encode_string
cbor_encode_string_array
cbor_encode_text_string
cbor_encode_text_stringz
cbor_encode_uint
and minicbor::encode::Encode
has all the equivalent methods.
@kkysen Nix support and offline builds are not something we're committed to. Unless, minicbor
offers a C/C++ API, I'm not sure it is a good replacement. Would adding tinycbor as a git submodule work? Let's discuss before you move ahead with this.
Making tinycbor
a git submodule might work (https://github.com/immunant/c2rust/issues/470#issuecomment-1172257062). But I don't think creating C bindings for minicbor
, specifically the type-specific ones in minicbor::encode::Encode<Vec<u8>>
to match exactly the ones we use listed above, would be that hard. This comes with a few extra benefits as well:
We don't have an assertion if the CBOR-encoded C AST is > 64 MBs because we use a growable Vec<u8>
as the output, not a (uint8_t *buffer, size_t size)
:
https://github.com/immunant/c2rust/blob/5d4e7a637196727eb6cf118628792d8b726dd556/c2rust-ast-exporter/src/AstExporter.cpp#L2604-L2615
We could also use minicbor
for deserializing from the Rust side instead of serde_cbor
. serde_cbor
is no longer maintained and suggests using either minicbor
or ciborium
. Of the two, minicbor
had a lot more downloads (and is no_std
-compatible, too). If we switch to using minicbor
on the Rust side, then we use the same library for serializing and deserializing, which has less of a chance for any bugs.
We use a Rust library instead of a C one. This integrates better with the rest of our Rust code and the community and its infrastructure (e.x. cargo
, docs.rs, etc.). And more importantly, it is safe. I'm not sure how widely used or safe tinycbor
is, but I know our clang
bindings and related code are unsound as they have data race bugs that can segfault.
The bindings we add for minicbor
could easily keep the exact same C API as tinycbor
except for maybe the errors, but we never check the error return values anyways. And we could potentially publish it and let it be used by others as well.
I think if making tinycbor
a git submodule is easy and makes offline builds work, we can do that first as it'd probably be faster, but I think the minicbor
bindings route is the better one, and if the git submodule doesn't work well we should do that instead (I haven't had the best experiences with git submodules in the past).
Building
tinycbor
by downloading it incmake
causes many issues:We should figure out how to build it offline or switch to a solution that can be.