r234591 - Initializing an uninitialized data member; should be NFC.

[Aaron Ballman]

On Fri, Apr 10, 2015 at 11:20 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Fri, Apr 10, 2015 at 8:06 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Fri, Apr 10, 2015 at 11:03 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >
>> >
>> > On Fri, Apr 10, 2015 at 6:05 AM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> Author: aaronballman
>> >> Date: Fri Apr 10 08:05:04 2015
>> >> New Revision: 234591
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=234591&view=rev
>> >> Log:
>> >> Initializing an uninitialized data member; should be NFC.
>> >
>> >
>> > Was this being used uninitialized? Where?
>> >
>> > It's actually rather helpful to keep uninitialized members
>> > uninitiialized if
>> > they aren't intended to be used, so MSan et, al, can catch their use.
>>
>> It was not, that I could tell. However, it was the only member to not
>> be initialized, and it is a public member of the class, which strikes
>> me as a dangerous code smell.
>
>
> The danger is rather mitigated by having tools like MSan.

Not everyone has those tools available to them, or uses them
regularly, so I'm not completely convinced that's a reasonable
mitigation in this case. I would agree it's a great last resort, but I
don't think it is sufficient for a *bool* (for which it is really easy
to get accidentally correct behavior from uninitialized variables).
Perhaps it's more sufficient for other types where it's less likely to
get correct behavior while debugging locally. But it is very annoying
to have locally "correct" code that winds up breaking bots and sends
out a ton of spam when that can be otherwise avoided.

> But in any case, there doesn't seem to be anything special about that member
> to warrant it being uninitialized while the others are initialized, as you
> say.
>
> I had thought this might be one of the cases of a discriminated union that
> I've seen in other places, where initializing the non-active members really
> hinders MSan's ability to check that union accesses all only access the
> active members. But looking at it more closely that doesn't seem to be the
> case here - it's just one of a bunch of similar members, nothing special.

That was the conclusion I came to as well. Plus, it looked like a red
herring while I was debugging something else. Every other member was
initialized except this one, so I spent time chasing down why that
was.

~Aaron