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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:52-54
+namespace {
+template <typename T> using Table = std::vector<std::vector<T>>;
+}
----------------
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.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:58
 
+enum class machine { UNKNOWN = 0, ARM, X64, X86 };
+
----------------
Enum classes must start with an uppercase letter. Also, `= 0` is redundant.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:71
+  uint16_t getMajorVersion() const {
+    return (maskLeadingOnes<uint32_t>(16) & Suffix->Version) >> 16;
+  }
----------------
You can just do

  return Suffix->Version >> 16;


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:74
+  uint16_t getMinorVersion() const {
+    return maskTrailingOnes<uint32_t>(16) & Suffix->Version;
+  }
----------------
and

  return Suffix->Version;


================
Comment at: llvm/lib/Object/WindowsResource.cpp:137
   while (!End) {
 
+    Data.emplace_back(Entry.getData());
----------------
Remove this blank line.


================
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;
+}
----------------
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.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:115-118
+  bool Verbose = false;
+
+  if (InputArgs.hasArg(OPT_VERBOSE))
+    Verbose = true;
----------------
  bool Verbose = InputArgs.hasArg(OPT_VERBOSE);


https://reviews.llvm.org/D34020