warn when ctor-initializer leaves uninitialized data?

[initializing a char array with strlcpy or strcpy]
[calling a helper method to initialize the members]
[using assignment statements in the body of the constructor]

This warning has the potential to get really messy if you try to
handle all possible cases.</stating-the-obvious> I know because I
implemented something exactly like this in (a proprietary fork of) the
EDG front-end a few years ago. Not saying you shouldn't try, just
saying it wasn't fun trying (and failing) to make it foolproof.

Here's one more semi-common idiom that we ran into at the time:

Foo::Foo() {
    memset(this, '\0', sizeof *this); // most of my fields should be
false or zero
    this->should_be_true = true; // initialize only the interesting ones
}

-Arthur

Perhaps that as a first approach it would be simpler to just not warn whenever there is at least one statement in the constructor’s body ?

This is also a common pattern in C++03 because of the absence of delegating constructors:

class A {
public:
A() { this->init(0); }
A(int x) { this->init(x); }

};

And the definition of “init” might not be available.

Thank you all for your responses and for the awesome feedback. I think there are a couple of different ideas in this thread. While it would be fantastic to try and find uninitialized values in arbitrary code, that would definitely be a much larger change and I don’t think I’m up for that yet. It looks like -Wuninitialized currently follows the flow of uninitialized variables within a function and could probably be modified to also follow members not in the initializer list through the constructor. However, following flow into other functions can be very complicated and, as Matthieu brought up, it isn’t possible in the general case because the source for some methods might not even be available. Such a diagnostic would end up being an approximation and would sometimes either have to reject valid code or silently ignore invalid code.

I guess the warning I was envisioning was more of a stylistic warning. The equivalent warning from gcc is -Weffc++ which includes warnings about violations of several style guidelines from Scott Meyers’ book “Effective C++, Second Edition” including this warning (always put all members in the initializer list). It includes a few other warnings and is helpful in preventing mistakes, especially for beginners, but generates a lot of noise when run on large existing projects and unfortunately can’t be split into separate, fine-grained warnings. The warning in this patch is much more targeted but makes the choice to reject some valid code instead of silently ignoring invalid code. It was more targeted for beginner to intermediate users of C++ and not for large, highly optimized projects. Maybe this wants to be two warnings, one for initializer lists as it is currently and one that attempts to trace uninitialized members to the end of each constructor but doesn’t warn when it isn’t sure?

  • Evan

I do think this should still go in, even if it's off by default. Any easy checks could be included, any difficult checks should not.

OTOH, we've run into things like this before, and the general rule has been that Clang is not in the habit of introducing new style warnings. The style warnings we have are generally ones that have historical precedent (-Wparentheses). So my personal positive preference may not match up with Clang's big-picture goals.

The one thing that would make it nicer is a way to silence the warning without initialization, but I can't think of a good way offhand.

My two cents,
Jordy

FYI:

Just this afternoon I activated a build with -Wall -Wextra -Werror on gcc and I had to add a -Wno-missing-initializer because gcc erroneously warns on:

struct tm timepoint = {};
timepoint.tm_year = 111;

saying that all members other than tm_year are not initialized. It is really annoying to put up with such false positives.

So I would be very interested in warning that is never wrong, and maybe Ted would approve of a specific -Wbeginner group to put warnings that are potentially wrong but really help beginners ?

– Matthieu

Having spent a little bit of time trying to track down a bug that was caused by unitialized POD data members, I’d also like to see your patch committed as an opt-in warning.

+1.

  • Patrick

When I use this patch, it breaks one of the other tests:

FAIL: Clang :: SemaCXX/default-constructor-initializers.cpp (3644 of 4580)
******************** TEST ‘Clang :: SemaCXX/default-constructor-initializers.cpp’ FAILED ********************
Script:

Wuninitialized-member.patch (4.1 KB)

Since I lost some hours last week to a constructor that forgot to initialize a certain field, I figured I’d give Matthieu’s suggestion of only warning for constructors with an empty body a shot. I couldn’t get it to work, even when I made the warning more and more restricted, using fairly desperate heuristics in the end.

  • Let’s only warn on constructors with empty bodies!
  • …but that warns on constructors that use member arrays as writable scratch buffers (used in protobuf), so only do this for scalar members
  • …but that warns on ParsedInternalKey() { } // Intentionally left uninitialized (for speed) constructors (e.g. leveldb), so only do this if an explicit init sequence is present
  • …but that warns on classes that intentionally initialize only 1-2 members and keep the rest uninitialized, so only warn if a constructor list initializes all but one member
  • …but that still warns all over the place, admittedly sometimes on code with questionable design (which however is still correct), so make it an opt-in warning and only enable it for “good” code
  • …but that warns if base class fields are protected and initialized in all subclasses (e.g. WebKit’s Vector), so only do this for private fields
  • …but even “high-quality” libraries (I think Chromium’s net/ library is generally pretty good, for example) still contain many instances where some fields aren’t initialized intentionally. Some of that code does look reasonable.

I did find some things that look like bugs during this exercise, but the false-positive rate remains very high and I’m out of ideas to make the warning more restricted.

I’m attaching my patch in case anyone wants to play with it (run it on LLVM instead of Chromium, for example). It includes a few interesting test cases too.

Nico

clang-ctor-list.diff (9.48 KB)