Hi,
I updated the patch on the bugzilla.
I think anonymous unions are now handled correctly.
I also added a test case (also included in the patch).
I still need to add the inheritance case.
If you think of any other cases I've missed that should be added to
the test case, let me know.
On Sun, 22 Nov 2015 at 00:22 Martin Sebor <msebor@xxxxxxxxx> wrote:
>
> On 11/19/2015 03:54 PM, Anthony Brandon wrote:
> > Hi,
> >
> > I'm not that familiar with GCCs trees structures and how to check for
> > these kinds of cases.
> > If you have any hints for me for where to look for examples, that
> > would be appreciated.
>
> I usually look for examples of how similar problems are handled
> elsewhere in the front end. But it seems to me that you're on
> the right track and might just need to tweak the code a bit.
> Some good old fashioned debugging (and lots of time and patience)
> should be all you need.
>
> > For the second case I'm guessing I'd have to somehow checked that the
> > constructor is actually taking the value and not the address,
> > but for the first case I'm not sure I can think of a way to check for
> > that in the general case.
>
> The first case is an instance of C anonymous structs and unions
> whose members need to be treated as if they were members of the
> containing struct. Your patch skips over them in the conditional
> below but does the right thing when the conditional is removed.
>
> /* Only look at the top most COMPONENT_REF to ignore members
> of parent classes or structs. */
> if (!(TREE_OPERAND (*tp, 0) == current_class_ref))
>
> Below is a slightly different example that the patch also misses
> because of the conditional. Since removing the conditional causes
> problems elsewhere, it looks to me like all you need to is find
> a way to distinguish those two classes of problems and handle each
> appropriately.
>
> struct C { int a; };
> struct D: C {
> int b;
> D ():
> b (a)
> { }
> };
>
> FWIW, as I think Manuel already suggested, creating a test and
> coming up with a bunch of test cases will help you find the
> shortcomings in your solution. Debugging them should then help
> you arrive at a more robust patch.
>
> Martin
>
> >
> > On Sat, Nov 14, 2015 at 6:12 PM, Martin Sebor <msebor@xxxxxxxxx> wrote:
> >> On 11/14/2015 03:00 AM, Anthony Brandon wrote:
> >>>
> >>> Hi Martin,
> >>>
> >>> I don't know how close my patch is to being finished.
> >>> But it seems like unless we need a separate warning for Winit_self, we
> >>> should probably just combine that code with mine and just give a
> >>> " 'i' is uninitialized " warning, rather than " 'i' is initialized
> >>> with itself ".
> >>> I've attached my patch here:
> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19808
> >>> If we need to detect multiple uninitialized values in an initializer
> >>> we need something other than walk_tree I think, since it just returns
> >>> the first match.
> >>> I'll submit it to gcc-patches for comments later.
> >>> What is EXPR_LOC_OR_LOC?
> >>>
> >>
> >> Sounds good.
> >>
> >> By the way, while testing your patch I found a couple of corner cases
> >> that gcc doesn't handle quite right. This one where the uninitialized
> >> member b is used to initialize a isn't diagnosed:
> >>
> >> struct B {
> >> union { int a; int b; };
> >> B ():
> >> a (b)
> >> { }
> >> };
> >>
> >> This one initializes a reference so it doesn't actually access the
> >> uninitialized member but is diagnosed with the patched gcc:
> >>
> >> struct A {
> >> int &r;
> >> int a;
> >> A ():
> >> r (a),
> >> a ()
> >> { }
> >> };
> >>
> >> EXPR_LOC_OR_LOC() is a macro just like EXPR_LOCATION() except that it
> >> takes a second argument, LOCATION, which is used when the expression
> >> doesn't have a location associated with it. The macro is defined in
> >> tree.h.
> >>
> >> Martin
> >>
> >>
> >>> On Fri, Nov 13, 2015 at 4:12 PM, Martin Sebor <msebor@xxxxxxxxx> wrote:
> >>>>
> >>>> On 11/13/2015 04:30 AM, Manuel López-Ibáñez wrote:
> >>>>>
> >>>>>
> >>>>> Hi Anthony,
> >>>>>
> >>>>> Would you mind attaching your draft patch to the PR? You could also
> >>>>> submit it to gcc-patches with a "[RFC, C++]" note in the subject to get
> >>>>> some early feedback on it.
> >>>>>
> >>>>> I think Martin Sebor has recently fixed the i(i) case (or improved it).
> >>>>
> >>>>
> >>>>
> >>>> I just posted a patch adjusting the location to point at the member
> >>>> being initialized:
> >>>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01420.html
> >>>> The patch hasn't been committed or even approved yet. It looks like
> >>>> Anthony's fixed it in his own patch so I might as well withdraw mine
> >>>> if you're close to being done. For reference, I also opened bug 68301
> >>>> for an outstanding problem in this area.
> >>>>
> >>>> For the case below, even though it might look like it will lead to
> >>>> a lot of duplicitous output, I would think that simply diagnosing
> >>>> every instance of using an uninitialized member would be fine in
> >>>> practice. That's what Clang does:
> >>>>
> >>>> u.cpp:5:18: warning: field 'j' is uninitialized when used here
> >>>> [-Wuninitialized]
> >>>> S() : i(j), j(1) {}
> >>>> ^
> >>>> u.cpp:11:18: warning: field 'j' is uninitialized when used here
> >>>> [-Wuninitialized]
> >>>> B() : i(j+i), j(j+1) {}
> >>>> ^
> >>>> u.cpp:11:20: warning: field 'i' is uninitialized when used here
> >>>> [-Wuninitialized]
> >>>> B() : i(j+i), j(j+1) {}
> >>>> ^
> >>>> u.cpp:11:26: warning: field 'j' is uninitialized when used here
> >>>> [-Wuninitialized]
> >>>> B() : i(j+i), j(j+1) {}
> >>>> ^
> >>>> u.cpp:17:18: warning: field 'i' is uninitialized when used here
> >>>> [-Wuninitialized]
> >>>> C() : i(i) {}
> >>>> ^
> >>>>
> >>>> Martin
> >>>>
> >>>>
> >>>>>
> >>>>> Cheers,
> >>>>>
> >>>>> Manuel.
> >>>>>
> >>>>> On 11/12/2015 10:12 PM, Anthony Brandon wrote:
> >>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> I found the code from when I worked on 19808.
> >>>>>> With this input:
> >>>>>>
> >>>>>> struct S
> >>>>>> {
> >>>>>> int i, j;
> >>>>>> S() : i(j), j(1) {}
> >>>>>> };
> >>>>>>
> >>>>>> struct B
> >>>>>> {
> >>>>>> int i, j;
> >>>>>> B() : i(j+i), j(j+1) {}
> >>>>>> };
> >>>>>>
> >>>>>> struct C
> >>>>>> {
> >>>>>> int i, j;
> >>>>>> C() : i(i) {}
> >>>>>> };
> >>>>>>
> >>>>>> I get this output:
> >>>>>>
> >>>>>> test.C:4:10: warning: ‘S::i’ is initialized with uninitialized field
> >>>>>> ‘S::j’ [-Wuninitialized]
> >>>>>> S() : i(j), j(1) {}
> >>>>>> ^
> >>>>>>
> >>>>>> test.C:10:10: warning: ‘B::i’ is initialized with uninitialized field
> >>>>>> ‘B::j’ [-Wuninitialized]
> >>>>>> B() : i(j+i), j(j+1) {}
> >>>>>> ^
> >>>>>>
> >>>>>> test.C:16:10: warning: ‘C::i’ is initialized with itself [-Winit-self]
> >>>>>> C() : i(i), j(1) {}
> >>>>>> ^
> >>>>>> The main questions I have are what to do in cases like
> >>>>>> i(i+j) and the like, or where multiple uninitialized values are used,
> >>>>>> or i(i+1) for that matter.
> >>>>>>
> >>>>>>
> >>>>>> On Tue, Nov 10, 2015 at 12:40 AM, Manuel López-Ibáñez
> >>>>>> <manuel.lopez-ibanez@xxxxxxxxxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 09/11/15 20:41, Zygmunt Ptak wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> Is there any param in the gcc which will warn about not initialized
> >>>>>>>> class member from the initialization list?
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> Unfortunately, no. We do not even warn for:
> >>>>>>>
> >>>>>>> struct S
> >>>>>>> {
> >>>>>>> int i, j;
> >>>>>>> S() : i(j), j(1) {}
> >>>>>>> }
> >>>>>>>
> >>>>>>> This is https://gcc.gnu.org/PR19808 and it should be not too
> >>>>>>> difficult to
> >>>>>>> fix, it just needs someone with enough time and perseverance to fix
> >>>>>>> it.
> >>>>>>> Anthony Brandon started working on it, but I'm not sure what is the
> >>>>>>> status
> >>>>>>> now. Of course, anyone is more than welcome to pick it up.
> >>>>>>>
> >>>>>>> There is also https://gcc.gnu.org/PR2972, which is probably closer to
> >>>>>>> what
> >>>>>>> you want. The current patch
> >>>>>>> (https://gcc.gnu.org/ml/gcc-patches/2011-11/msg01068.html) will warn
> >>>>>>> even if
> >>>>>>> the member is initialized within the constructor. But if this is what
> >>>>>>> you
> >>>>>>> want, you could try updating the patch to the latest trunk, complete
> >>>>>>> it and
> >>>>>>> submit it for approval.
> >>>>>>>
> >>>>>>> Cheers,
> >>>>>>>
> >>>>>>> Manuel.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >
> >
> >
>
