[PATCH] D55417: [clangd] Change disk-backed storage to be atomic

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 17 04:28:31 PST 2018

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits

Comment at: clangd/index/BackgroundIndexStorage.cpp:39
+template <typename WriterFunction>
+llvm::Error writeAtomically(llvm::StringRef OutPath, WriterFunction &&Writer) {
+  // Write to a temporary file first.
NIT: use `llvm::function_ref` here. It forces to clearly state the signature of the function and allows to avoid templates.

Comment at: clangd/index/BackgroundIndexStorage.cpp:44
+  auto EC = llvm::sys::fs::createUniqueFile(
+      llvm::Twine(OutPath, ".tmp.%%%%%%%%"), FD, TempPath);
+  if (EC)
NIT: I believe there's an overloaded `operator +(StringRef, const char*)` that would produce a Twine, so maybe simplify to `OutPath + ".tmp.%%%%%"`?
Just a suggestion, up to you.

Comment at: clangd/index/BackgroundIndexStorage.cpp:60
+  // If everything went well, we already moved the file to another name. So
+  // don't delete it, as it might be taken by another file.
+  RemoveOnFail.release();
NIT: `s/it/the name/`

  rCTE Clang Tools Extra



More information about the cfe-commits mailing list