<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h">File include/osmocom/core/stat_item.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@42">Patch Set #1, Line 42:</a> <code style="font-family:monospace,monospace">    int32_t not_stats_next_id;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Since it seems nobody is using this field outside libosmocore (you are changing the time), then prob […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh whoops i didn't mean to rename the member at all.<br>The "not_" was just a temporary trick to catch all occurrences...</p><p style="white-space: pre-wrap; word-wrap: break-word;">The remaining question is do we drop these unused items because we assume they should never be used outside of libosmocore?<br>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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@49">Patch Set #1, Line 49:</a> <code style="font-family:monospace,monospace">        struct osmo_stat_item_period value;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm really not understanding fully why you leave the other fields while changing their name. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(was a mistake to push those)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@125">Patch Set #1, Line 125:</a> <code style="font-family:monospace,monospace">        OSMO_DEPRECATED("Access item->value.last, item->value.min, item->value.max or osmo_stat_item_get_avg() instead");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'd add API getter for last, min and max if they are to be used outside libosmocore (I doubt so).</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@127">Patch Set #1, Line 127:</a> <code style="font-family:monospace,monospace">void osmo_stat_item_flush(struct osmo_stat_item *item);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">There's a new function in between deprecated ones? please move it somewhere else.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/25464">change 25464</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/25464"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I137992a5479fc39bbceb6c6c2af9c227bd33b39b </div>
<div style="display:none"> Gerrit-Change-Number: 25464 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 15 Sep 2021 17:38:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>