[PATCH] D129117: ManagedStatic: eliminate uses for cl::opt in the llvm directory

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Support/Timer.cpp:47
-// causing havoc to ensue.  We "fix" this by creating the string the first time
-// it is needed and never destroying it.
-static ManagedStatic<std::string> LibSupportInfoOutputFilename;
----------------
nhaehnle wrote:
> efriedma wrote:
> > Is the problem described by this comment not an issue anymore?  I guess the timing of the call to initTimerOptions is more predictable since we aren't using a global variable, but is it guaranteed to happen before the StatisticInfo constructor runs?
> I don't think so, and haven't seen any test issues.
> 
> The theoretical justification is: previously, the string was owned by LibSupportInfoOutputFilename, which is seeded from initTimerOptions via getOptions.
> 
> With this change, it is owned directly by Options::InfoOutputFilename, which is also seeded from initTimerOptions via getOptions.
> 
> Previously, there were multiple globals that had to be coordinated somehow. With the change, everything is owned by a single global.
> 
> Now that said, revisiting this I just realized that there is a real change here, which is that if CreateInfoOutputFile is called *without* first calling initTimerOptions, the change will now implicitly create the cl::opts, which was not the case previously.
> 
> Is that a problem? Calling CreateInfoOutputFile without first initializing this module seems like a weird thing to do.
The issue would be if the StatisticInfo destructor runs after the string is destroyed.  To avoid this, we need to ensure the "Options" struct is constructed before the StatisticInfo constructor returns.

> Now that said, revisiting this I just realized that there is a real change here, which is that if CreateInfoOutputFile is called *without* first calling initTimerOptions, the change will now implicitly create the cl::opts, which was not the case previously.
>
>Is that a problem? Calling CreateInfoOutputFile without first initializing this module seems like a weird thing to do.

This doesn't seem likely to cause a problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129117/new/

https://reviews.llvm.org/D129117