[PATCH] D79066: [Support] Class to facilitate file locking

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 3 inline comments as done.
sepavloff added a comment.

@labath, thank you for the review!
Maybe you could look at the patch D78896 <https://reviews.llvm.org/D78896> on which this patch depends?



================
Comment at: llvm/include/llvm/Support/FileSystem.h:1181
+public:
+  FileLocker(int FD) : FD(FD) {}
+  FileLocker(const FileLocker &L) = delete;
----------------
labath wrote:
> If this stays as public, I think it should be documented very explicitly that this takes an already-locked file descriptor. Alternatively, we could still introduce the static factory function here (:P) to do the locking, but have the raw_ostream function as the "recommended" way of locking a raw_fd_ostream (actually, without exposing the file descriptor, it will essentially be the _only_ way of doing it).
As @dblaikie proposed earlier, this constructor is now made private.


================
Comment at: llvm/unittests/Support/Path.cpp:2174
+    EXPECT_THAT_EXPECTED(L, Failed());
+    handleAllErrors(std::move(L.takeError()), [&](ErrorInfoBase &EIB) {});
+  }
----------------
labath wrote:
> This line here is not going anything because the EXPECT macro already "consumes" the error.  The double consumes for Errors are mostly harmless in practice, but I already got complaints from some clang-tidy checks about this, when I accidentally introduced them. So... consider yourself warned. :)
> 
> I know you want to demonstrate the "recommended" usage. That is commendable, but I don't think it really works out here...
Indeed, thank you for for the catch. Avoid using `EXPECT_THAT_EXPECTED` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79066