[PATCH] D62718: [llvm-objcopy] Change handling of output file permissions

Mon Jun 24 01:57:57 PDT 2019

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test:4
+
+## Unix version of this test must use umask(1)
+## or tests might fail.
----------------
"The Unix version..."

Why might the tests fail? Be a bit more explicit (specifically, they might fail because the user has a different umask to the build bots).

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-win.test:4
+
+# REQUIRES: system-windows
+
----------------
jhenderson wrote:
> This should have a comment explaining why this test is Windows-specific.
Still missing this comment... It'll largely be the same as the Linux one, just inverted.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mirror-permissions.test:1
+# RUN: touch %t
+# RUN: chmod 0777 %t
----------------
I think you accidentally re-added this file?

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

https://reviews.llvm.org/D62718