<p><a href="https://gerrit.asterisk.org/10602">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c">File main/backtrace.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.asterisk.org/#/c/10602/6/main/backtrace.c@33">Patch Set #6, Line 33:</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;">#include "asterisk/utils.h"<br>#include "asterisk/strings.h"<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These includes are unused and should be removed.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@58">Patch Set #6, Line 58:</a> <code style="font-family:monospace,monospace">#include "asterisk/vector.h"</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">My vote is to prevent vector.h from including asterisk/lock.h.  We<br>should avoid as many Asterisk headers as possible.<br>/* Block include of asterisk/lock.h */<br>#define _ASTERISK_LOCK_H</p><p style="white-space: pre-wrap; word-wrap: break-word;">On the other hand maybe vector.h shouldn't include asterisk/lock.h<br>at all.  Any source which uses locking versions of vectors should<br>directly include asterisk/lock.h.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I'm of the opinion that files, whether source or header, should include the things they need directly.  That way users of header files don't need to worry about what they might need to also include.  In any case, I disabled the lock.h inclusion.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@111">Patch Set #6, Line 111:</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;">/*<br> * We CANNOT call the ast_ equivalents for pthread_<br> * functions here so we need to undo the redirections<br> * created by lock.h so we can call the pthread_<br> * functions directly.<br> */<br>#undef pthread_mutex_lock<br>#undef pthread_mutex_unlock<br>#undef pthread_mutex_t<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I'd prefer this be moved to the top after including headers.  If we<br>remove the include of lock.h from vector.h this will become<br>unnecessary.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@163">Patch Set #6, Line 163:</a> <code style="font-family:monospace,monospace">             snprintf(data->msg, MSG_BUFF_LEN, "%s %s %s:%u %s()",</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">In this example output the first [%p] of each line is from<br>ast_log_backtrace, the second [%p] is from right here.  Probably<br>should just print the address from here instead of<br>ast_log_backtrace, I don't think ast_log_backtrace can compensate<br>for the inline frames that do not have corresponding entries in the<br>backtrace addresses.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I didn't even notice the duplicate pointers.  Removed from ast_log_backtrace.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@305">Patch Set #6, Line 305:</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;">struct ast_vector_string *__ast_bt_get_symbols(void **addresses, size_t num_frames)<br>{<br>       return __ast_bt_get_symbols_impl(addresses, num_frames);<br>}<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why not just rename __ast_bt_get_symbols_impl in the two sections?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10602">change 10602</a>. To unsubscribe, or for help writing mail filters, 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/10602"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I79d02862ddaa2423a0809caa4b3b85c128131621 </div>
<div style="display:none"> Gerrit-Change-Number: 10602 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 13 Nov 2018 14:23:14 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>