There was a chance that multiple clangd instances could try to write
same shard, in which case we would get a malformed file most likely. This patch
changes the writing mechanism to first write to a temporary file and then rename
it to fit real destination. Which is guaranteed to be atomic by POSIX.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 25819 Build 25818: arc lint + arc unit
Event Timeline
NIT: I believe we're missing a hyphen in the change description, i.e. it should be disk-backed.
clangd/index/BackgroundIndexStorage.cpp | ||
---|---|---|
98 | Maybe extract this into a helper function? std::error_code writeAtomically(StringRef OutPath, function_ref<void(raw_ostream&)> Writer) { /// create temp file, open output stream. Writer(OS): /// Move the temp file into OutPath. } This would keep the storeShard function as readable as it is now | |
100 | Maybe create these temporary files in the same dir? That would ensure the move operations do not require transferring data between physical devices. |
LGTM with a few nits
clangd/index/BackgroundIndexStorage.cpp | ||
---|---|---|
39 | NIT: use llvm::function_ref here. It forces to clearly state the signature of the function and allows to avoid templates. | |
44 | NIT: I believe there's an overloaded operator +(StringRef, const char*) that would produce a Twine, so maybe simplify to OutPath + ".tmp.%%%%%"? | |
60 | NIT: s/it/the name/ |
NIT: use llvm::function_ref here. It forces to clearly state the signature of the function and allows to avoid templates.