r235190 - Remove the assertion as it was useless and broken.

Jordan Rose jordan_rose at apple.com
Wed Apr 29 09:01:51 PDT 2015


> On Apr 29, 2015, at 5:31 , Aaron Ballman <aaron at aaronballman.com <mailto:aaron at aaronballman.com>> wrote:
> 
> On Tue, Apr 28, 2015 at 11:59 PM, Jordan Rose <jordan_rose at apple.com <mailto:jordan_rose at apple.com>> wrote:
>> 
>> On Apr 28, 2015, at 20:57 , Jordan Rose <jordan_rose at apple.com <mailto:jordan_rose at apple.com>> wrote:
>> 
>> 
>> On Apr 17, 2015, at 6:37 , Aaron Ballman <aaron at aaronballman.com <mailto:aaron at aaronballman.com>> wrote:
>> 
>> On Fri, Apr 17, 2015 at 9:35 AM, Sylvestre Ledru <sylvestre at debian.org <mailto:sylvestre at debian.org>>
>> wrote:
>> 
>> On 17/04/2015 15:33, Aaron Ballman wrote:
>> 
>> On Fri, Apr 17, 2015 at 9:21 AM, Sylvestre Ledru <sylvestre at debian.org <mailto:sylvestre at debian.org>>
>> 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 <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.

Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150429/810c316c/attachment.html>


More information about the cfe-commits mailing list