<div dir="ltr">Not yet... I was holding off on submitting, looking for feedback on my reading of Type.6 discussed in my previous email from March 14th. I'm ready to post it for review whenever, though (though I need to figure out how to do that...)<div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 28, 2016 at 2:04 PM, Felix Berger <span dir="ltr"><<a href="mailto:flx@google.com" target="_blank">flx@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Is there an outstanding review I can take a look at?</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 28, 2016 at 4:47 PM, Michael Miller <span dir="ltr"><<a href="mailto:michael_miller@motu.com" target="_blank">michael_miller@motu.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Any thoughts on this before I submit what I've got for review? I've now run it against our entire codebase and it seems to be behaving as expected.<div><br></div><div>Thanks!</div><span><font color="#888888"><div>Michael Miller</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 14, 2016 at 9:04 AM, Michael Miller <span dir="ltr"><<a href="mailto:michael_miller@motu.com" target="_blank">michael_miller@motu.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks, Felix and Alex! I'm basically done updating the check and expanding the tests.<div><br></div><div>One issue I've run into is that if you have multiple constructors on a class, it's very possible to get warnings about initializing the same member in each. In that case, we end up adding multiple sets of initialization braces for the in-class initializer. I'm not sure the best way to deduplicate those. clang-tidy will handle it if the warnings are identical but that's not really possible here while also pointing out the missing members in individual constructors.</div><div><br></div><div>The other issue/decision has more to do with the specific wording of the C++ Core Guideline, Type.6, that I alluded to in the first email. Types with non-trivial default constructors present a slight conundrum, e.g.:</div><span><div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">struct Foo {</div><div style="font-size:12.8px">  int Bar;</div><div style="font-size:12.8px">  std::string Baz;</div><div style="font-size:12.8px">};</div></div><div><br></div></span><div>I see three choices that will defend against uninitialized values.</div><div>1. Add a default constructor that initializes Bar.</div><div>2. Initialize Bar in-class (C++11 only).</div><div>3. Initialize Foo wherever it's used.</div><div><br></div><div>1 and 2 seem to more closely match the wording of Type.6, unless we interpret "trivially constructible type" to mean any type without a user-provided default constructor.</div><div><br></div><div>Should I submit what I've got and we can discuss there or is it worth addressing these questions before I do that?</div><div><br></div><div>Thanks!</div><span><font color="#888888"><div>Michael Miller</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 2, 2016 at 8:57 AM, Felix Berger <span dir="ltr"><<a href="mailto:flx@google.com" target="_blank">flx@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Michael,<div><br></div><div>thanks for looking into this! Feel free to improve the existing check. While I would like to work on it more I'll be busy the next couple of weeks and the existing check already addresses the main issue I've seen in my code base.</div><div><br></div><div>Cheers,</div><div>Felix</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 2, 2016 at 10:54 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">The author of the check (cc-ed) had plans to work on it further, IIUC.<div><br></div><div>-- Alex</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 26, 2016 at 8:49 PM, Michael Miller via cfe-dev <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hi all!</div><div><br></div><div>I just finished writing a clang-tidy check to flag uninitialized members variables. I was thus very happy to see the cppcoreguidelines-pro-type-member-init check land in clang-tidy mainline recently! The documentation for this clang-tidy check implies it's targeting "Type.6: Always initialize a member variable." It seems like some aspects might be missing, though, and I was wondering if there's anyone working on adding them or if the plan is to keep the current check as is.</div><div><br></div><div>Type.6's enforcement section reads:</div><div>• Issue a diagnostic for any constructor of a non-trivially-constructible type that does not initialize all member variables. To fix: Write a data member initializer, or mention it in the member initializer list.</div><div>• Issue a diagnostic when constructing an object of a trivially constructible type without () or {} to initialize its members. To fix: Add () or {}.</div><div><br></div><div>Right now, the clang-tidy check only seems to address the first bullet point yet only partially. It flags member variables that are built-ins or pointers. Based on my reading, it needs to flag any trivially constructible typed members, not just built-ins and pointers, excepting fully in-class initialized record types. If the second bullet point were addressed, either in this check or a separate one, the two together should cover the full text.</div><div><br></div><div>The other thing I wonder about is whether trivially constructible is too narrow. There are types that are not trivially constructible, at least in the definition that I know, that still should be flagged:</div><div><br></div><div>struct Foo {</div><div>  int Bar;</div><div>  std::string Baz;</div><div>};</div><div><br></div><div>Foo is not trivially constructible because Baz has a user defined constructor, yet a default initialized Foo will leave Bar in an indeterminate state. Therefore, it seems like we need to initialize any member that is one of the following:</div><div>A. Built-in or pointer type</div><div>B. Record type without a user-provided default constructor with at least one member that needs to be initialized as per these criteria</div><div>C. Array of any type matching these criteria*</div><div><br></div><div>Is anyone actively working on these aspects of Type.6 or is that something that I should look into?</div><div><br></div><div>* One might prefer arrays to be default initialized for performance reasons in some cases. Perhaps this should be an option.</div><div><br></div><div>Thanks!</div><span><font color="#888888"><div>Michael Miller</div></font></span></div>
<br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>