[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/WindowsResource.h:61
+
+  static inline bool classof(const Binary *V) { return V->isWinRes(); }
+
----------------
Remove `inline` as it is redundant.  When you write a function definition inside class declaration, it is inline by default.


================
Comment at: llvm/test/tools/llvm-cvtres/resource.test:5-6
+
+RUN: llvm-cvtres %p/Inputs/test_resource.res \
+RUN:   | FileCheck %s
+
----------------
Now you can write this in a single line.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:72
+  errs() << Msg;
+  errs().flush();
+  exit(1);
----------------
I think err is unbuffered, so you don't need flush().


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:81
+void error(std::error_code EC) {
+  if (!EC) return;
+  reportError(EC.message() + ".\n");
----------------
Can you run clang-format-diff? There are many formatting errors (in terms of the LLVM coding style) in this file.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.h:15
+
+void error(std::error_code EC);
+
----------------
ecbeckmann wrote:
> zturner wrote:
> > Can you put this in the same file as the other enum?  That said, it looks like maybe it is only used from 1 file?  In that case make it static inline in the file where you need it.
> In future patches this function will surely be used in multiple files.
Add this header in the future patch which needs this header. Adding a stub is usually fine, but this file seems too stub-y.


https://reviews.llvm.org/D33180