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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129117