[PATCH] D78903: [Driver] Add option -fproc-stat-report

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked 2 inline comments as done.
sepavloff added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:3814
+          llvm::raw_fd_ostream OS(StatReportFile, EC, llvm::sys::fs::OF_Append);
+          if (!EC) {
+            if (auto L = OS.tryToLock())
----------------
aganea wrote:
> sepavloff wrote:
> > aganea wrote:
> > > If the goal is to report accurate information, maybe it's worth looping here a bit in case of an error, to give the chance to other clang.exe instances to release the file lock? What do you think? (same for `tryToLock`)
> > Actually this method makes blocking request and waits infinitely.
> > 
> > There is long discussion of how this lock helper must be implemented: https://reviews.llvm.org/D79066. Finally the variant with timeout was removed, only blocking one remained. The method has prefix `try` because it requires handling result value.
> > 
> > If you think the name or interface might be better, you can participate in the discussion in D79066, that patch isn't committed yet.
> Thanks!
> 
> In the same way as above, I think the code will read better if it did early exits. Also, I don't think you need `handleAllErrors`, you could replace it with `toString` and report the error code the user:
> ```
> if (EC)
>   return;
> auto L=OS.tryToLock();
> if (!L) {
>   llvm::errs() ... << toString(L.takeError());
>   return;
> }
> OS << Buffer;
> ```
It becomes better. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78903