<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8366">View Change</a></p><p>Patch set 1:</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">(1 comment)<br>> I have not done a complete review yet but if this eliminates the<br>ABI difference then MALLOC_DEBUG should be added to the list of<br>exceptions in build_tools/make_buildopts_h.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">The intent is to eliminate the API differences between MALLOC_DEBUG<br>being enabled and it being disabled.  So yes I'll need to add it to<br>the exceptions.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Once the make_buildopts_h change is made I can +1.</p><p>(4 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/astmm.h">File include/asterisk/astmm.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/astmm.h@174">Patch Set #1, Line 174:</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;">#else<br>#error "NEVER INCLUDE astmm.h DIRECTLY!!"<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Nit: this can be removed as it will be harmless to directly include astmm.h.  Feel free to ignore this comment if you wish.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/hashtab.h">File include/asterisk/hashtab.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/hashtab.h@261">Patch Set #1, Line 261:</a> <code style="font-family:monospace,monospace">#define ast_hashtab_create(initial_buckets, compare, resize, newsize, hash, do_locking) \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thank you!  Really hate when macro's have generic a,b,c,d argument names, makes IDE tool-tips useless.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/stringfields.h">File include/asterisk/stringfields.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/stringfields.h@231">Patch Set #1, Line 231:</a> <code style="font-family:monospace,monospace">    /* v-- MALLOC_DEBUG information */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is unfortunate.  I know it's just 2 pointers and an int but we use string fields in a lot of structures.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Alternative: What if we updated __ast_string_field_alloc_space, __ast_string_field_ptr_build and __ast_string_field_ptr_build_va to use the callers file/line/func?  Be using the ast_string_field_mgr owner_* we're actually obscuring situations where we allocate additional space.  If we could see which locations cause additional space to be allocated this might allow us to make minor changes to optimize memory usage (like maybe the original allocation size should exclude fields that will always be rewritten).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Feel free to defer or even refuse this comment.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/utils.h">File include/asterisk/utils.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/utils.h@536">Patch Set #1, Line 536:</a> <code style="font-family:monospace,monospace">#define ast_free free</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You did notice the comment about 10 lines above right?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see now. please disregard my previous comments at this location.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8366">change 8366</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/8366"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ic07ad80b2c2df894db984cf27b16a69383ce0e10 </div>
<div style="display:none"> Gerrit-Change-Number: 8366 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 22 Feb 2018 19:12:47 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>