On Wed, Apr 29, 2015 at 12:01 PM, Jordan Rose <[email protected]> wrote: > > On Apr 29, 2015, at 5:31 , Aaron Ballman <[email protected]> wrote: > > On Tue, Apr 28, 2015 at 11:59 PM, Jordan Rose <[email protected]> wrote: > > > On Apr 28, 2015, at 20:57 , Jordan Rose <[email protected]> wrote: > > > On Apr 17, 2015, at 6:37 , Aaron Ballman <[email protected]> wrote: > > On Fri, Apr 17, 2015 at 9:35 AM, Sylvestre Ledru <[email protected]> > wrote: > > On 17/04/2015 15:33, Aaron Ballman wrote: > > On Fri, Apr 17, 2015 at 9:21 AM, Sylvestre Ledru <[email protected]> > wrote: > > Author: sylvestre > Date: Fri Apr 17 08:21:39 2015 > New Revision: 235190 > > URL: http://llvm.org/viewvc/llvm-project?rev=235190&view=rev > Log: > Remove the assertion as it was useless and broken. > > Enforcing the assert caused the following tests to fail: > Clang :: Analysis__bstring.c > Clang :: Analysis__comparison-implicit-casts.cpp > Clang :: Analysis__malloc-interprocedural.c > Clang :: Analysis__malloc.c > Clang :: Analysis__redefined_system.c > Clang :: Analysis__string.c > Clang :: Analysis__weak-functions.c > > > While the assert may have been broken, I am concerned that the > author's assumptions are being violated in some way. Can the original > code author weigh in on whether that assert is truly useless or not? > That appears to be Jordan in this case, according to a quick svn > blame. > > Yes, sorry about that. I fixed it quickly and maybe not using the best way. > However, the incorrect assertion (fixed r235188) has been there for a few > years. > > > I agree that this is an improvement over broken bots and an assert > that behaves differently in debug vs release mode. ;-) I just want to > make sure we double-check that this is the right move long-term. > > > I wrote this a long time ago, but the intent was that you wouldn't get to > this point without setting CurrentFunctionDescription for your specific > caller. I haven't actually looked at how it's being called now, though, > since CStringChecker is a beta checker. > > > Ah. The code as written originally was correct: > > // Make sure each function sets its own description. > // (But don't bother in a release build.) > assert(!(CurrentFunctionDescription = nullptr)); > > > ...except that the canonical way to write this is > > #ifndef NDEBUG > CurrentFunctionDescription = nullptr; > #endif > > > and apparently past me didn't know that. > > > Do you think we should add the code back in (under the canonical > spelling), or do you think it's fine to leave it removed? > > > 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.
Friendly ping on this, Sylvestre, so it doesn't fall off your radar. ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
