[PATCH] D34020: Implement COFF emission for parsed Windows Resource ( .res) files.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann removed a reviewer: hiraditya.
ecbeckmann added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:52-54
+namespace {
+template <typename T> using Table = std::vector<std::vector<T>>;
+}
----------------
ruiu wrote:
> I don't think it is a good practice to declare a new type in a header in an anonymous namespace because (I believe it is intended to be an internal data type but) it is visible from everywhere that includes this file. You can just use `std::vector<std::vector<T>>`, no? It is actually more readable than `Table<T>` as the former type is obvious.
Okay, whichever is most readable.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:387-392
+static std::time_t getTime() {
+  std::time_t Now = time(nullptr);
+  if (Now < 0 || !isUInt<32>(Now))
+    return UINT32_MAX;
+  return Now;
+}
----------------
ruiu wrote:
> I doubt you actually want to fill the timestamp field of the output file. If you do that, any build is no longer reproducible. You probably want to leave it zero.
Hmm isn't the timestamp important for incremental linking to work?  I don't think it's important for builds to be reproducible, since we are testing our output with the llvm-readobj tool.


https://reviews.llvm.org/D34020