Patch Set 1: Code-Review+1 (6 comments)
I'd +2 after some feedback and that minor formatting fix https://gerrit.osmocom.org/#/c/4336/1/tests/Makefile.am File tests/Makefile.am: Line 14: bits/bitfield_test \ (though I dislike it,) the scheme here is to line up the '\', which you're breaking https://gerrit.osmocom.org/#/c/4336/1/tests/bits/bitfield_test.c File tests/bits/bitfield_test.c: Line 10: #define INTRO(p) do { printf("=== start %s(%u) ===\n", __func__, p); } while(0) heh, if it's one function call, you might as well just omit the semicolon instead of putting a do{}while around it #define FOO(x) printf("foo") Line 15: bool use_lh) this looks to me like it is imitating a "real" function from libosmocore. Is there one? Will it become one? Then we should call that "real" function instead to test it? Line 25: bitvec_write_field(dest, &wp, 3, 2); /* "HH" */ both if paths call identical code? Is it your way of what I did in that osmo-hlr patch with those '#if 0' that I couldn't uphold against your valid arguments? :) Indeed in the test output, both invocations do the same. >From the future: if a following patch changes this, please say so in the >commit log or in the fixme comment so I don't spend time trying to figure it >out. Line 44: bitvec_write_field(dest, &wp, 0, 1); /* BEP_PERIOD2 not present */ sure are a lot of comments for a 'deadbeef8002231' hex result :) Line 142: printf("\n\n"); does it make sense to move the line feeds into the OUTRO macro? -- To view, visit https://gerrit.osmocom.org/4336 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ee544256b8675bc62a42493aab66a8eeee54f90 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Minh-Quang Nguyen <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-HasComments: Yes
