<p>Kevin Harwell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8604">View Change</a></p><p>Patch set 7:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(4 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c">File main/data_buffer.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@103">Patch Set #7, Line 103:</a> <code style="font-family:monospace,monospace">buffer->max - buffer->count</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">minor, but since this is calculated multiple times in this function you could do it once at the beginning. Also the assigned variable name could give descriptive meaning to the calculation.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@112">Patch Set #7, Line 112:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">                      buffer_payload = data_buffer_payload_alloc(NULL, -1);<br>                 if (buffer_payload) {<br>                         AST_LIST_INSERT_TAIL(&buffer->cached_payloads, buffer_payload, list);<br>                          buffer->cache_count++;<br>                     }<br><br>                   i++;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If the entry does not get inserted into the cache then "i" still gets incremented, thus potentially not adding the desired number of entries. Might be easiest to just drop the "i" usage and key off "cache_count" only?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@125">Patch Set #7, Line 125:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">                      buffer_payload = AST_LIST_REMOVE_HEAD(&buffer->cached_payloads, list);<br>                 if (buffer_payload) {<br>                         ast_free(buffer_payload);<br>                             buffer->cache_count--;<br>                     }<br><br>                   i--;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Similar here, to the above. Unlikely, but possible for the number of items removed to be less than expected potentially resulting in a cache > max_size.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/7/main/data_buffer.c@178">Patch Set #7, Line 178:</a> <code style="font-family:monospace,monospace">                       if (remove) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would only remove items from the cache if the current cache size is greater than size. Mind as well keep already allocated cached objects unless the max_size of the buffer shrinks to less than the current cache size.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8604">change 8604</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.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.asterisk.org/8604"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Iff13c5d4795d52356959fe2a57360cd57dfade07 </div>
<div style="display:none"> Gerrit-Change-Number: 8604 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Matthew Fredrickson <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 23 Mar 2018 16:30:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>