ilya-biryukov added inline comments.
================
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > Maybe use `unique_ptr<Tracer>` (or even `llvm::Optional<Tracer>`)?
> > Otherwise we leak memory and don't flush the stream if users of tracing API
> > forget to call `stop()`. (I think I'm on the same page with @ioeric here
> > about forgetting a call to `stop()`).
> >
> > Having a static global of non-pod type is a no-go, but we could use static
> > local variables:
> >
> > ```
> > llvm::Optional<Tracer>& getTracerInstance() {
> > static llvm::Optional<Tracer> Tracer;
> > return Tracer;
> > }
> > ```
> So Eric's solution of a scoped object that would be owned by main() makes
> sense to me, and I'm happy to add it (though personally not sure it carries
> its weight).
>
> However I don't understand *your* suggestion :-)
> > Having a static global of non-pod type is a no-go, but we could use static
> > local variables:
> > llvm::Optional<Tracer>& getTracerInstance() {
> > static llvm::Optional<Tracer> Tracer;
> > return Tracer;
> > }
> So it's not global, which means we're not calling Optional() on startup,
> which would actually be totally safe (I guess the constructor should be
> marked as `constexpr`).
>
> But we *are* calling `~Optional()` and therefore `~Tracer()` at shutdown. And
> maybe `~Tracer()` calls something that logs, and llvm::errs() has already
> been destroyed... *this* is why avoid static objects, right?
>
> > Otherwise we leak memory and don't flush the stream if users of tracing API
> > forget to call stop()
> The intended time to call `stop()` is at the end of the program. If you
> forget to call it, we won't be leaking memory :-) We indeed won't flush the
> stream, but that wouldn't be safe (see above).
>
> The other option is the user is calling `start()` and `stop()` multiple
> times, and they call `start()` twice without stopping. We already assert on
> this, and anyone who doesn't reproducibly hit that assert is well into racy
> UB-land (logging methods may be called concurrently with each other, but
> everything else must be synchronized).
>
> Overall I'm not really seeing what scenario we can detect and safely do
> something about.
Forget what I said, you're right. Tracer has state, can be reconfigured
multiple times and there's no guarantee our references won't be dangling on
destruction of `Tracer`.
I do think Eric's solution is good, though. For all the same reasons why RAII
is good.
https://reviews.llvm.org/D39086
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
