[cfe-dev] cppcoreguidelines-pro-type-member-init

[Michael Miller via cfe-dev]

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...)

On Mon, Mar 28, 2016 at 2:04 PM, Felix Berger <flx at google.com> wrote:

> Is there an outstanding review I can take a look at?
>
> On Mon, Mar 28, 2016 at 4:47 PM, Michael Miller <michael_miller at motu.com>
> wrote:
>
>> 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.
>>
>> Thanks!
>> Michael Miller
>>
>> On Mon, Mar 14, 2016 at 9:04 AM, Michael Miller <michael_miller at motu.com>
>> wrote:
>>
>>> Thanks, Felix and Alex! I'm basically done updating the check and
>>> expanding the tests.
>>>
>>> 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.
>>>
>>> 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.:
>>>
>>> struct Foo {
>>>   int Bar;
>>>   std::string Baz;
>>> };
>>>
>>> I see three choices that will defend against uninitialized values.
>>> 1. Add a default constructor that initializes Bar.
>>> 2. Initialize Bar in-class (C++11 only).
>>> 3. Initialize Foo wherever it's used.
>>>
>>> 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.
>>>
>>> Should I submit what I've got and we can discuss there or is it worth
>>> addressing these questions before I do that?
>>>
>>> Thanks!
>>> Michael Miller
>>>
>>> On Wed, Mar 2, 2016 at 8:57 AM, Felix Berger <flx at google.com> wrote:
>>>
>>>> Hi Michael,
>>>>
>>>> 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.
>>>>
>>>> Cheers,
>>>> Felix
>>>>
>>>> On Wed, Mar 2, 2016 at 10:54 AM, Alexander Kornienko <alexfh at google.com
>>>> > wrote:
>>>>
>>>>> The author of the check (cc-ed) had plans to work on it further, IIUC.
>>>>>
>>>>> -- Alex
>>>>>
>>>>> On Fri, Feb 26, 2016 at 8:49 PM, Michael Miller via cfe-dev <
>>>>> cfe-dev at lists.llvm.org> wrote:
>>>>>
>>>>>> Hi all!
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Type.6's enforcement section reads:
>>>>>> • 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.
>>>>>> • Issue a diagnostic when constructing an object of a trivially
>>>>>> constructible type without () or {} to initialize its members. To fix: Add
>>>>>> () or {}.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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:
>>>>>>
>>>>>> struct Foo {
>>>>>>   int Bar;
>>>>>>   std::string Baz;
>>>>>> };
>>>>>>
>>>>>> 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:
>>>>>> A. Built-in or pointer type
>>>>>> B. Record type without a user-provided default constructor with at
>>>>>> least one member that needs to be initialized as per these criteria
>>>>>> C. Array of any type matching these criteria*
>>>>>>
>>>>>> Is anyone actively working on these aspects of Type.6 or is that
>>>>>> something that I should look into?
>>>>>>
>>>>>> * One might prefer arrays to be default initialized for performance
>>>>>> reasons in some cases. Perhaps this should be an option.
>>>>>>
>>>>>> Thanks!
>>>>>> Michael Miller
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-dev mailing list
>>>>>> cfe-dev at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160328/0a8e2c99/attachment.html>