<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, May 17, 2018 at 1:55 AM Hans Wennborg via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">hans added inline comments.<br>
<br>
<br>
================<br>
Comment at: lld/COFF/Driver.cpp:1045<br>
+      if (Value.getAsInteger(0, Config->Timestamp))<br>
+        fatal("invalid timestamp: " + Value);<br>
+    }<br>
----------------<br>
Maybe the error message could be clearer, indicating that it expects an integer?<br>
<br>
<br>
================<br>
Comment at: lld/COFF/Options.td:61<br>
 def subsystem : P<"subsystem", "Specify subsystem">;<br>
+def timestamp : P<"timestamp", "Specify the timestamp to be written to the PE header">;<br>
 def version : P<"version", "Specify a version number in the PE header">;<br>
----------------<br>
Perhaps just the shorter "Specify the PE header timestamp" is enough?<br>
<br>
<br>
================<br>
Comment at: lld/test/COFF/timestamp.test:9<br>
+RUN: llvm-readobj -file-headers -coff-debug-directory %t.3.exe | FileCheck %s --check-prefix=ZERO<br>
+<br>
+HASH: ImageFileHeader {<br>
----------------<br>
Should there be a test for the default case, without /Brepro or /timestamp?<br></blockquote><div><br></div><div>I thought about it, but I'm not sure how to write a test case for that, since the timestamp will be whatever the time is when the test is run. </div><div><br></div><div>I'll land it now to unblock the roll, and if anyone comes up with a good way to write a test case for the default, I'll add that as a followup.</div></div></div>