Change in libosmocore[master]: msgb: add test helper

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Fri Nov 30 11:44:47 UTC 2018


Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12017 )

Change subject: msgb: add test helper
......................................................................


Patch Set 1: Code-Review-1

(7 comments)

https://gerrit.osmocom.org/#/c/12017/1/include/osmocom/core/msgb.h
File include/osmocom/core/msgb.h:

https://gerrit.osmocom.org/#/c/12017/1/include/osmocom/core/msgb.h@569
PS1, Line 569: bool msgb_cmpr(const char *f, size_t l, const struct msgb *msg, const uint8_t *data, size_t len, bool print);
strcmp, so why not msgb_cmp instead of msgb_cmpr?


https://gerrit.osmocom.org/#/c/12017/1/src/msgb.c
File src/msgb.c:

https://gerrit.osmocom.org/#/c/12017/1/src/msgb.c@176
PS1, Line 176: static inline bool compare_n_print(const char *f, size_t l, uint8_t level,
remove inline, let the compiler decide what's more optimal.


https://gerrit.osmocom.org/#/c/12017/1/src/msgb.c@196
PS1, Line 196: 	/* N. B: that's intended to be used bymsgb_cmpr*() so length check is already passed,
This comment needs to be moved to line 180 since len is already used there.


https://gerrit.osmocom.org/#/c/12017/1/src/msgb.c@230
PS1, Line 230: /*! Compare and print: check data in msgb against given data and print erros if any
typo: errors


https://gerrit.osmocom.org/#/c/12017/1/src/msgb.c@231
PS1, Line 231:  *  \param[in] f text prefix, usually __func__, ignored if print == false
If it's based on print value, then it makes sense to move it at the end after print, not here at the start of the function.

BTW, you should move these functions to have an underscore before it (_msg_cmp) and then have a define as msg_cmp(foo, bar) _msg_cmp(foo, bar, __FILE__, __LINE__).

We do that in fsm.h check it.


https://gerrit.osmocom.org/#/c/12017/1/src/msgb.c@232
PS1, Line 232:  *  \param[in] l numeric prefix, usually __LINE__, ignored if print == false
Same, move at the end.


https://gerrit.osmocom.org/#/c/12017/1/src/msgb.c@239
PS1, Line 239: bool msgb_cmpr(const char *f, size_t l, const struct msgb *msg, const uint8_t *data, size_t len, bool print)
I'd expect a msgb_cmp to compare two structs of msgb, so better calling this one msgb_cmp_data, in case we want to add the other one later.

Now that we are adding the API, what about having it return an int like memcmp? after all we are using memcmp, so we can return an int.



-- 
To view, visit https://gerrit.osmocom.org/12017
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3bc95f2f5ab6e3f4b502647fb3e0aaaf1f7c4cf5
Gerrit-Change-Number: 12017
Gerrit-PatchSet: 1
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Fri, 30 Nov 2018 11:44:47 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181130/bc5715e0/attachment.htm>


More information about the gerrit-log mailing list