Thank you for the review!
On Thu, May 30, 2019 at 12:38 PM Marek Polacek <pola...@redhat.com> wrote:
>
> Thanks for the patch! Do you have a copyright assignment on file? Unsure
> if this patch is small enough not to need it.
>
Also unsure, though I assumed that this patch was small enough to not need one.
I'll go ahead and do it regardless.
> > gcc/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie <s...@swgillespie.me>
> >
> > * doc/invoke.texi: Add new flag -Wglobal-constructors.
> >
> > gcc/c-family/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie <s...@swgillespie.me>
> >
> > * c.opt: Add new flag -Wglobal-constructors.
> >
> > gcc/cp/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie <s...@swgillespie.me>
> >
> > * decl.c (expand_static_init): Warn if a thread local or static decl
> > requires a non-trivial constructor or destructor.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie <s...@swgillespie.me>
> >
> > * g++.dg/warn/global-constructors-1.C: New test.
> > * g++.dg/warn/global-constructors-2.C: New test.
>
> These are fine but please also mention "PR c++/71482" in them.
>
Will fix.
> > ---
> > gcc/c-family/c.opt | 4 +++
> > gcc/cp/decl.c | 17 ++++++++++---
> > gcc/doc/invoke.texi | 7 ++++++
> > .../g++.dg/warn/global-constructors-1.C | 25 +++++++++++++++++++
> > .../g++.dg/warn/global-constructors-2.C | 23 +++++++++++++++++
> > 5 files changed, 73 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-1.C
> > create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-2.C
> >
> > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> > index 046d489f7eb..cf62625ca42 100644
> > --- a/gcc/c-family/c.opt
> > +++ b/gcc/c-family/c.opt
> > @@ -613,6 +613,10 @@ Wformat-truncation=
> > C ObjC C++ LTO ObjC++ Joined RejectNegative UInteger
> > Var(warn_format_trunc) Warning LangEnabledBy(C ObjC C++ LTO
> > ObjC++,Wformat=, warn_format >= 1, 0) IntegerRange(0, 2)
> > Warn about calls to snprintf and similar functions that truncate output.
> >
> > +Wglobal-constructors
> > +C++ ObjC++ Var(warn_global_constructors) Warning
> > +Warn when a global declaration requires a constructor to initialize.
> > +
>
> Ok, I agree this shouldn't be turned on by -Wall or -Wextra.
Yeah - the way I use this with clang is to forbid global constructors
with -Werror=global-constructors and -Wglobal-constructors. Since
this is a legitimate language feature I don't think it should be
included in either of those catch-all warning sets.
>
> > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> > index 19d14a6a5e9..c1d66195bd7 100644
> > --- a/gcc/cp/decl.c
> > +++ b/gcc/cp/decl.c
> > @@ -8467,10 +8467,21 @@ expand_static_init (tree decl, tree init)
> > finish_then_clause (if_stmt);
> > finish_if_stmt (if_stmt);
> > }
> > - else if (CP_DECL_THREAD_LOCAL_P (decl))
> > - tls_aggregates = tree_cons (init, decl, tls_aggregates);
> > else
> > - static_aggregates = tree_cons (init, decl, static_aggregates);
> > + {
> > + if (CP_DECL_THREAD_LOCAL_P (decl))
> > + tls_aggregates = tree_cons (init, decl, tls_aggregates);
> > + else
> > + static_aggregates = tree_cons (init, decl, static_aggregates);
> > +
> > + if (warn_global_constructors)
>
> This check is not wrong but you can probably drop it. We use it if the
> warning
> is expensive, but this doesn't seem to be the case.
>
Will fix.
> > + {
> > + if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (decl)))
> > + warning(OPT_Wglobal_constructors, "declaration requires a global
> > destructor");
> > + else
> > + warning(OPT_Wglobal_constructors, "declaration requires a global
> > constructor");
> > + }
> > + }
>
> The formatting a bit wrong, please use two spaces to indent. There should be
> a space before a (. And the lines should not exceed 80 characters.
>
> I think it would be better to use warning_at instead of warning. As the
> location, you can use 'dloc' defined earlier in the function if you move
> it out of the block.
>
Will fix.
>
> It seems this patch will also warn for
>
> struct A {
> A() = default;
> };
>
> A a;
>
> which I think we don't want?
>
> I also think you want to add a test using a constexpr constructor.
>
> Marek
You're completely right. I have a modified version of this patch that
expands the test coverage to include these cases while also
handling the general problem of constant and non-constant
initializers. I'll submit it shortly.
Sean Gillespie
