<div dir="ltr">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.<div><br></div><div>* Let's only warn on constructors with empty bodies!</div><div>* …but that warns on constructors that use member arrays as writable scratch buffers (used in protobuf), so only do this for scalar members</div><div>* …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</div><div>* …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</div><div>* …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</div><div>* …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</div><div>* …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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Nico</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 27, 2012 at 11:38 AM, Patrick Beard <span dir="ltr"><<a href="mailto:beard@apple.com" target="_blank">beard@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><br><div><div>On Apr 23, 2012, at 11:33 AM, Patrick Beard <<a href="mailto:beard@apple.com" target="_blank">beard@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word"><br><div><div>On Apr 10, 2012, at 1:22 PM, Evan Wallace wrote:</div><br><blockquote type="cite"><span style="border-collapse:separate;font-family:Helvetica;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;font-size:medium"><div>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?</div></span></blockquote></div><br><div>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.</div></div></blockquote><br></div></div></div><div>When I use this patch, it breaks one of the other tests:</div><div><br></div><div><div>FAIL: Clang :: SemaCXX/default-constructor-initializers.cpp (3644 of 4580) </div><div>******************** TEST 'Clang :: SemaCXX/default-constructor-initializers.cpp' FAILED ******************** </div><div>Script: </div><div>-- </div><div>/private/var/tmp/compiler.build/Release+Asserts/bin/clang -cc1 -internal-isystem /private/var/tmp/compiler.build/Release+Asserts</div><div>/bin/../lib/clang/3.2/include -fsyntax-only -verify /private/var/tmp/compiler/llvm/tools/clang/test/SemaCXX/default-constructor-</div><div>initializers.cpp </div><div>--</div><div>Exit Code: 1</div><div>Command Output (stderr):</div><div>--</div><div>error: 'note' diagnostics expected but not seen: </div><div> Line 54: first required here</div><div>1 error generated.</div><div><br></div><div>Here's the patch I'm using:</div><div><br></div><div></div></div></div><br><div style="word-wrap:break-word"><div><div></div><div><br></div><div>If I comment out the change to BuildImplicitMemberInitializer(), I confirm that the output of the default-constructor-initializers.cpp test differs by this line:</div><div><br></div><div><div>/Trees/compiler/llvm/tools/clang/test/SemaCXX/default-constructor-initializers.cpp:54:4: note: implicit default constructor for 'Z1' first required here</div><div>Z1 z1; // expected-note {{first required here}}</div><div> ^</div></div><div><br></div><div>That is, this note appears if the change is commented out, but isn't there if:</div><div><br></div><div><div style="margin:0px;font-family:Menlo;font-size:11px;color:rgb(0,132,0)"><span style="color:#000000"> </span>// Warn if ctor-initializer for a non-POD type leaves uninitialized data.</div><div style="margin:0px;font-family:Menlo;font-size:11px"> <span style="color:#bb2ca2">if</span> (!Constructor->getParent()->isPOD()) {</div><div style="margin:0px;font-family:Menlo;font-size:11px"> SemaRef.Diag(Constructor->getLocation(), diag::warn_uninit_member)</div><div style="margin:0px;font-family:Menlo;font-size:11px"> << Field->getDeclName();</div><div style="margin:0px;font-family:Menlo;font-size:11px"> }</div></div><div><br></div><div>is included.</div><div><br></div><div>- Patrick</div><div><br></div></div></div><br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>