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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
