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

Mon Mar 28 14:25:44 PDT 2016

Alright! I will do that later today.

Thanks!
Michael Miller

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

> If you've already written one version of it I'd say put it up for review.
> This makes it concrete and is no extra work if you post it as is.
>
> On Mon, Mar 28, 2016 at 5:07 PM, Michael Miller <michael_miller at motu.com>
> wrote:
>
>> 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/e885e1bf/attachment.html>