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

[Pavel Labath via Phabricator via llvm-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

This looks good to me, modulo a couple of inline comments. But let's give a chance for @dblaikie to look this over too...



================
Comment at: llvm/include/llvm/Support/FileSystem.h:1181
+public:
+  FileLocker(int FD) : FD(FD) {}
+  FileLocker(const FileLocker &L) = delete;
----------------
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).


================
Comment at: llvm/lib/Support/Error.cpp:85-90
+std::string ErrorInfoBase::message() const {
+  std::string Msg;
+  raw_string_ostream OS(Msg);
+  log(OS);
+  return OS.str();
+}
----------------
I'd move this next to the definitions of other ErrorInfoBase members (line 53--54).


================
Comment at: llvm/unittests/Support/Path.cpp:2174
+    EXPECT_THAT_EXPECTED(L, Failed());
+    handleAllErrors(std::move(L.takeError()), [&](ErrorInfoBase &EIB) {});
+  }
----------------
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79066