On Thu, May 30, 2019 at 05:28:47PM -0600, Martin Sebor wrote:
> On 5/28/19 3:30 PM, Sean Gillespie wrote:
> > This adds a new warning, -Wglobal-constructors, that warns whenever a
> > decl requires a global constructor or destructor. This new warning fires
> > whenever a thread_local or global variable is declared whose type has a
> > non-trivial constructor or destructor. When faced with both a constructor
> > and a destructor, the error message mentions the destructor and is only
> > fired once.
> >
> > This warning mirrors the Clang option -Wglobal-constructors, which warns
> > on the same thing. -Wglobal-constructors was present in Apple's GCC and
> > later made its way into Clang.
> >
> > Bootstrapped and regression-tested on x86-64 linux, new tests passing.
>
> I can't tell from the Clang online manual:
>
> Is the warning meant to trigger just for globals, or for all
> objects with static storage duration regardless of scope (i.e.,
> including namespace-scope objects and static locals and static
> class members)?
>
> "Requires a constructor to initialize" doesn't seem very clear
> to me. Is the warning intended to trigger for objects that
> require dynamic initialization? If so, then what about dynamic
> intialization of objects of trivial types, such as this:
>
> static int i = std::string ("foo").length ();
>
> or even
>
> static int j = strlen (getenv ("TMP"));
The warning is not meant to diagnose these. But I do agree that the
documentation for the new warning should be improved.
> If these aren't meant to be diagnosed then the description should
> make it clear (the first one involves a ctor, the second one does
> not). But I would think that if the goal is to find sources of
> dynamic initialization then diagnosing the above would be useful.
> If so, the description should make it clear and tests verifying
> that it works should be added.
>
> Martin
>
> PS Dynamic initialization can be optimized into static
> initialization, even when it involves a user-defined constructor.
> If the purpose of the warning is to find objects that are
> dynamically initialized in the sense of the C++ language then
> implementing it in the front-end is sufficient. But if the goal
My sense is that this is what we're doing here. So the patch seems reasonable
to me. I suspect we'll have to tweak the warning a little bit, but overall it
looks fine to me. As always, there's room to expand and improve, but the
warning looks useful to me as-is.
Marek
