[PATCH] D33180: Add functionality to cvtres to parse all entries in res file.
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 19 14:19:32 PDT 2017
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
More information about the llvm-commits
mailing list