TLV generation cleanup and testing #3297
Conversation
cdecker
changed the title
cdecker
force-pushed
the
tlv-gen-cleanup
branch
2 times, most recently
from
e9b22e4 to
087546d
Compare
|
Ready for review :-) Ping @niftynei |
cdecker
force-pushed
the
tlv-gen-cleanup
branch
from
087546d to
fff4099
Compare
cdecker
force-pushed
the
tlv-gen-cleanup
branch
from
43b0599 to
f64ab69
Compare
There was a problem hiding this comment.
Just one trivial comment, this is great. Turned out really nice!
Ack f64ab69
tools/gen/header_template
Outdated
| */ | ||
| int ${tlv.name}_is_valid(const struct ${tlv.struct_name()} *record); | ||
| bool ${tlv.name}_is_valid(const struct ${tlv.struct_name()} *record, | ||
| int *err_index); |
cdecker
force-pushed
the
tlv-gen-cleanup
branch
from
f64ab69 to
b346c5b
Compare
|
Rebased on top of ACK 7e18a12 |
cdecker
force-pushed
the
tlv-gen-cleanup
branch
from
b346c5b to
83693bc
Compare
| SUPERVERBOSE("Unknown even type in TLV"); | ||
| return i; | ||
| SUPERVERBOSE("unknown even"); | ||
| if (err_index != NULL) |
There was a problem hiding this comment.
is this better than if (err_index) ?
There was a problem hiding this comment.
I slightly prefer it since it makes it clearer that we aren't checking a boolean but rather a pointer. I'm pretty sure it results in the same assembly.
There was a problem hiding this comment.
Indeed: https://godbolt.org/z/tcXEAc
There was a problem hiding this comment.
personally, i prefer the more concise if (err_index); the fact that err_index's type is a pointer should be clue enough that we're checking for pointer presence
I got this one wrong myself, since the function name implied a boolean result. So I changed it to take the optional err_index as argument.
We are now using the typesafe variant everywhere.
This is the counterpart to the typesafe deserialization function implemented in an earlier commit.
cdecker
force-pushed
the
tlv-gen-cleanup
branch
from
83693bc to
7e18a12
Compare
As promised in #3278 I went back and modified the
run-tlvstreamtests to use the typesafe TLV parser. Those last uses removed we could also remove the non-typesafe variantfromwire_tlvs. I'm planning to do the same for thetowire_tlvshelper and make that typesafe (hence the draft status).Changelog-None