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

[Aaron Ballman]

On Wed, Apr 29, 2015 at 12:01 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> On Apr 29, 2015, at 5:31 , Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Tue, Apr 28, 2015 at 11:59 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
>
> On Apr 28, 2015, at 20:57 , Jordan Rose <jordan_rose at apple.com> wrote:
>
>
> On Apr 17, 2015, at 6:37 , Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Fri, Apr 17, 2015 at 9:35 AM, Sylvestre Ledru <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>
> 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