[PATCH] D59033: [llvm-objcopy] Make .build-id linking atomic

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Missing context is making it hard for me to review the Path changes.



================
Comment at: llvm/include/llvm/Support/FileSystem.h:770
+/// Generates a unique path suitable for a temporary file but does not
+/// open or create the file. The name is based on \a model with '%'
+/// replaced by a random char in [0-9a-f]. If \a MakeAbsolute is true
----------------
model -> Model, I assume to match the variable name?


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:186
+  // partition on the same device which is critical. It has the added
+  // win of yet further decreasing the odds a conflict.
+  sys::fs::createUniquePath(Twine(Path) + "-" + MODEL_32 + ".tmp", TmpPath,
----------------
odds a conflict -> odds of a conflict


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:195
+  // We then atomically rename the link into place which will just move the
+  // link. If rename fails something is more seriouslly wrong so just return
+  // an error.
----------------
seriouslly -> seriously


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:202
+  }
+  // If `Path` was already a hard-link to he same underlying file then the
+  // temp file will be left so we need to remove it. Remove will not cause
----------------
"to he same"?

Not sure if that should be "to some" or "to the same"


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59033