[PATCH] D33180: Add functionality to cvtres to parse all entries in res file.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/Object/Binary.h:137
 
+  bool isRes() const { return TypeID == ID_WinRes; }
+
----------------
isWinRes


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:1-2
+//===-- WindowsResource.h -----------------------------------------------*- C++
+//-*-===//
+//
----------------
Format.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:58
+class WindowsResource : public Binary {
+ public:
+  ~WindowsResource() override;
----------------
Format?


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:59
+ public:
+  ~WindowsResource() override;
+
----------------
Do you need this?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:47-48
+        object_error::invalid_file_type);
+  std::unique_ptr<WindowsResource> Ret(new WindowsResource(Source));
+  return std::move(Ret);
+}
----------------
`return make_unique<WindowsResource>(Source)`?


================
Comment at: llvm/test/tools/llvm-cvtres/resource.test:5-6
+
+RUN: llvm-cvtres %p/Inputs/test_resource.res \
+RUN:   | FileCheck %s -check-prefix TEST_RES
+
----------------
Since you have only one test, you can omit `-check-prefix`, and s/TEST_RES/CHECK/. ("CHECK" is the default name.)


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:162
+    WindowsResource *RF = dyn_cast<WindowsResource>(&Binary);
+    if (!RF) reportError(File, cvtres_error::unrecognized_file_format);
+
----------------
You defined `cvtres_error` just for this line? If so, it seems too much to me. You could just do `reportError("Unsupported machine architecture")`.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:184
+  }
+  outs() << "\n";
   return 0;
----------------
If you add `\n` at end of all the above strings, you can remove this line. This is shorter and probably better

  if (Machine == machine::ARM)
    outs() << "ARM\n";
  else if (Machine == machine::X86")
    outs() << "X86\n";
  else
   outs() << "X64"\n";


https://reviews.llvm.org/D33180