<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464">View Change</a></p><p>8 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 style="white-space: pre-wrap; word-wrap: break-word;">Since it seems nobody is using this field outside libosmocore (you are changing the time), then probably change it to something even more understandable, like "reserved0", "reserved1", or alike.</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@46">Patch Set #1, Line 46:</a> <code style="font-family:monospace,monospace">        struct osmo_stat_item_value not_values[0];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This one can be dropped since it adds no space, and you are changing the name so it means nobody is using it outside libosmocore.</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 style="white-space: pre-wrap; word-wrap: break-word;">I'm really not understanding fully why you leave the other fields while changing their name. Can you explain the motivation or rationale behind keeping them? Is this struct used directly (not as a pointer) or any of its fields somewhere outside libosmocore?</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 style="white-space: pre-wrap; word-wrap: break-word;">I'd add API getter for last, min and max if they are to be used outside libosmocore (I doubt so).</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 style="white-space: pre-wrap; word-wrap: break-word;">There's a new function in between deprecated ones? please move it somewhere else.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/25464/1/src/stat_item.c">File src/stat_item.c:</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/src/stat_item.c@68">Patch Set #1, Line 68:</a> <code style="font-family:monospace,monospace"> * "virtual-trunk-0" and can be looked up by that name instead of its more or less arbitrary group index number).</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe add that user of this subsystem is in charge of avoiding group name collisions, ie assigning uniques names to different group instances.</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/src/stat_item.c@346">Patch Set #1, Line 346:</a> <code style="font-family:monospace,monospace">int osmo_stat_item_get_next(const struct osmo_stat_item *item, int32_t *next_id,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">next_id is not being used here, but documentation still explains old behavior. IIRC this is marked as deprecated right? Is it used by anyone anymore? maybe also write it here somewhere.</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/src/stat_item.c@369">Patch Set #1, Line 369:</a> <code style="font-family:monospace,monospace">    item->value.min = item->value.max = item->value.last;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure whether this is really what's needed here. Do we care at all in case value n=0 anyway? Maybe call osmo_stat_item_reset?</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-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 15 Sep 2021 14:10:24 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>