
<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Apr 29, 2015, at 5:31 , Aaron Ballman <<a href="mailto:aaron@aaronballman.com" class="">aaron@aaronballman.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On Tue, Apr 28, 2015 at 11:59 PM, Jordan Rose <</span><a href="mailto:jordan_rose@apple.com" style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">jordan_rose@apple.com</a><span style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">> wrote:</span><br style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="">On Apr 28, 2015, at 20:57 , Jordan Rose <<a href="mailto:jordan_rose@apple.com" class="">jordan_rose@apple.com</a>> wrote:<br class=""><br class=""><br class="">On Apr 17, 2015, at 6:37 , Aaron Ballman <<a href="mailto:aaron@aaronballman.com" class="">aaron@aaronballman.com</a>> wrote:<br class=""><br class="">On Fri, Apr 17, 2015 at 9:35 AM, Sylvestre Ledru <<a href="mailto:sylvestre@debian.org" class="">sylvestre@debian.org</a>><br class="">wrote:<br class=""><br class="">On 17/04/2015 15:33, Aaron Ballman wrote:<br class=""><br class="">On Fri, Apr 17, 2015 at 9:21 AM, Sylvestre Ledru <<a href="mailto:sylvestre@debian.org" class="">sylvestre@debian.org</a>><br class="">wrote:<br class=""><br class="">Author: sylvestre<br class="">Date: Fri Apr 17 08:21:39 2015<br class="">New Revision: 235190<br class=""><br class="">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=235190&view=rev" class="">http://llvm.org/viewvc/llvm-project?rev=235190&view=rev</a><br class="">Log:<br class="">Remove the assertion as it was useless and broken.<br class=""><br class="">Enforcing the assert caused the following tests to fail:<br class="">Clang :: Analysis__bstring.c<br class="">Clang :: Analysis__comparison-implicit-casts.cpp<br class="">Clang :: Analysis__malloc-interprocedural.c<br class="">Clang :: Analysis__malloc.c<br class="">Clang :: Analysis__redefined_system.c<br class="">Clang :: Analysis__string.c<br class="">Clang :: Analysis__weak-functions.c<br class=""><br class=""><br class="">While the assert may have been broken, I am concerned that the<br class="">author's assumptions are being violated in some way. Can the original<br class="">code author weigh in on whether that assert is truly useless or not?<br class="">That appears to be Jordan in this case, according to a quick svn<br class="">blame.<br class=""><br class="">Yes, sorry about that. I fixed it quickly and maybe not using the best way.<br class="">However, the incorrect assertion (fixed r235188) has been there for a few<br class="">years.<br class=""><br class=""><br class="">I agree that this is an improvement over broken bots and an assert<br class="">that behaves differently in debug vs release mode. ;-) I just want to<br class="">make sure we double-check that this is the right move long-term.<br class=""><br class=""><br class="">I wrote this a long time ago, but the intent was that you wouldn't get to<br class="">this point without setting CurrentFunctionDescription for your specific<br class="">caller. I haven't actually looked at how it's being called now, though,<br class="">since CStringChecker is a beta checker.<br class=""><br class=""><br class="">Ah. The code as written originally was correct:<br class=""><br class=""> // Make sure each function sets its own description.<br class=""> // (But don't bother in a release build.)<br class=""> assert(!(CurrentFunctionDescription = nullptr));<br class=""><br class=""><br class="">...except that the canonical way to write this is<br class=""><br class="">#ifndef NDEBUG<br class="">CurrentFunctionDescription = nullptr;<br class="">#endif<br class=""><br class=""><br class="">and apparently past me didn't know that.<br class=""></blockquote><br style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Do you think we should add the code back in (under the canonical</span><br style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">spelling), or do you think it's fine to leave it removed?</span><br style="font-family: Monaco; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote></div><br class=""><div class="">Yes, it should be there—way back when I was working on CStringChecker, it actually caught cases where I forgot to set the current function name. It could even just be there unconditionally, as long as it keeps the comment stating that it's for assertion purposes. I'm not sure why I was microoptimizing a single member access.</div><div class=""><br class=""></div><div class="">Jordan</div></body></html>