aganea added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:3782
+ = Cmd.getProcessStatistics();
+ if (ProcStat) {
+ if (PrintProcessStat) {
----------------
sepavloff wrote:
> aganea wrote:
> > In the case where `!ProcStat`, I am wondering if we shouldn't emit zero
> > values, in the report file at least. Otherwise if an invocation fails, it
> > won't be there in the report and we might wonder why. Emitting 0 might
> > indicate that something went wrong.
> The feature allows getting statistics using conventional tools like `make`.
> In this case the absence of numbers for a particular file is not an issue,
> maybe the file has already been built. If something goes wrong, clang must
> handle it in `Compilation::ExecuteCommand` and react properly. The fact of
> failed invocation would be handled by build tool. For the purpose of
> monitoring performance numbers it is more convenient to record successful
> compilations only.
>
>
>
Ok, thanks for clarifying! Could you please do an early exit here? ie.
```
if (!ProcStat)
return;
```
================
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())
----------------
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;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78903/new/
https://reviews.llvm.org/D78903
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
