Change in libosmocore[master]: refactor stat_item: get rid of FIFO and "skipped" error

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/.

neels gerrit-no-reply at lists.osmocom.org
Wed Sep 15 17:38:34 UTC 2021


neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/25464 )

Change subject: refactor stat_item: get rid of FIFO and "skipped" error
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h 
File include/osmocom/core/stat_item.h:

https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@42 
PS1, Line 42: 	int32_t not_stats_next_id;
> Since it seems nobody is using this field outside libosmocore (you are changing the time), then prob […]
oh whoops i didn't mean to rename the member at all.
The "not_" was just a temporary trick to catch all occurrences...

The remaining question is do we drop these unused items because we assume they should never be used outside of libosmocore?
Technically we guarantee that older code is able to compile against newer libosmocore, so those items should remain present (we always avoid API breakage). But in practice I agree that no-one is using it nor should be using it, so in practical terms we could break with that guarantee and just drop those unused members... that's a discussion on the level of release principles. Opinions welcome. These members have no use and no longer any meaning, simply there to avoid compilation errors, just in case some third party has code accessing them; of course such code would no longer work now either; so maybe would be nicer to indeed throw a compilation error instead of keeping the meaningless members?

We don't need 'reserved' placeholders, because ABI breakage is not the point at all. There is ABI breakage, period, recompiling is necessary. We want to avoid errors when compiling.


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@49 
PS1, Line 49: 	struct osmo_stat_item_period value;
> I'm really not understanding fully why you leave the other fields while changing their name. […]
(was a mistake to push those)


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@125 
PS1, Line 125: 	OSMO_DEPRECATED("Access item->value.last, item->value.min, item->value.max or osmo_stat_item_get_avg() instead");
> I'd add API getter for last, min and max if they are to be used outside libosmocore (I doubt so).
in the original stats API, the idea was to allow multiple consumers (hence the FIFO buffer in the first place), but we now assume the only consumer is the stats.c stats reporter. Also any user can simply access item->value.{last,min,max} directly, adding API for that would be Java-esque bloat IMO and not needed.

Well, if struct osmo_stat_item were opaque (not defined in the .h file), then this API would be nice to have, and we would also never have API breakage in the struct definition. But every time I authored opaque structs so far it ended up being a real bloat pain, most of the times i later refactored for transparent structs. So my opinion is to not add API to return members of transparent structs, for me is just bloat.


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@127 
PS1, Line 127: void osmo_stat_item_flush(struct osmo_stat_item *item);
> There's a new function in between deprecated ones? please move it somewhere else.
it's semantically related to the "discard" stuff, so I thought putting the replacement just above the deprecated functions it is replacing would be a courtesy to header file readers.



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

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I137992a5479fc39bbceb6c6c2af9c227bd33b39b
Gerrit-Change-Number: 25464
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 15 Sep 2021 17:38:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210915/a73f718c/attachment.htm>


More information about the gerrit-log mailing list