[PATCH] D50744: [llvm-strip] Add support for -p/--preserve-dates

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/strip-preserve-time.test:1
+# Note: ls -lu prints the accessed timestamp, ls -l prints the modified timestamp
+
----------------
Have you run this test both on Windows and Linux to ensure the they work the same? I know a colleague had some issues with access times on Windows at one point, for example.


================
Comment at: test/tools/llvm-objcopy/strip-preserve-time.test:3
+
+# Preserve dates when stripping to an output file
+# RUN: yaml2obj %s > %t.o
----------------
The comments in the file should end in a full stop, I think.


================
Comment at: test/tools/llvm-objcopy/strip-preserve-time.test:5
+# RUN: yaml2obj %s > %t.o
+# RUN: touch -a -t 199505050555.55 %t.o
+# RUN: touch -m -t 199705050555.55 %t.o
----------------
I'm slightly worried that this might lead to a flaky test, because there's a race condition here: if another process accesses the file some time between file creation and checking, then we'll presumably get a failure (and I could see a virus scanner doing just that...).


================
Comment at: test/tools/llvm-objcopy/strip-preserve-time.test:14
+# RUN: touch -m -t 199705050555.55 %t.o
+# RUN: llvm-objcopy -p %t.o -o %t-preserved.o
+# RUN: (ls -lu %t-preserved.o; ls -l %t-preserved.o) | FileCheck %s --check-prefix=CHECK-PRESERVE
----------------
I'd prefer it if you use a different output file for each case. It'll make it easier to debug any issues with failing tests, because all of the different cases will be available.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:680
 
+void restoreDateOnFile(StringRef Filename, const sys::fs::file_status &Stat) {
+  int FD;
----------------
Should this be static?


Repository:
  rL LLVM

https://reviews.llvm.org/D50744