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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I'd recommend renaming `ResFile` `WindowsRes` or `WindowsResource` everywhere, as I don't think Res or ResFile sound very familiar even for those who have an experience of Windows development, and for those who don't know about it, `Res` has no evidence about what that is.



================
Comment at: llvm/include/llvm/Object/Binary.h:60
 
+    ID_RES,
+
----------------
You want to add comment that this is Windows Resource, just like above enums.

Maybe ID_WINDOWS_RESOURCE is better? ID_RES seems too short.


================
Comment at: llvm/include/llvm/Object/ResFile.h:10
+//
+// This file declares the .res file class.
+//
----------------
the Windows .res, as Unix developers usually don't know what .res is.


================
Comment at: llvm/include/llvm/Object/ResFile.h:25
+namespace llvm {
+
+namespace object {
----------------
nit: remove


================
Comment at: llvm/include/llvm/Object/ResFile.h:28-30
+#define RETURN_IF_ERROR(X)                                                     \
+  if (auto EC = errorToErrorCode(X))                                           \
+    return EC;
----------------
You are using this macro only three times in this file, so it is better not to use it. In general you want to avoid using C macros, particularly in header files as it could conflict with .cpp files using the same identifier (unless you #undef the macro at end of the header.)


================
Comment at: llvm/include/llvm/Object/ResFile.h:45
+
+  std::error_code moveNext(bool &End);
+
----------------
I believe we should use llvm::Error instead of std::error_code for new code.


================
Comment at: llvm/include/llvm/Object/ResFile.h:51
+  const ResFile *OwningRes = nullptr;
+
+};
----------------
remove


================
Comment at: llvm/include/llvm/Object/ResFile.h:56
+public:
+  ResFile(MemoryBufferRef Source, Error &Err);
+  ~ResFile() override;
----------------
Looks like you want to allow users instantiate this class only with createResFile(). If so, it's better to make the constructor private.


================
Comment at: llvm/include/llvm/Object/ResFile.h:59
+
+  ResEntryRef HeadEntry();
+
----------------
Functions should start with a verb. Maybe getHeadEntry?


================
Comment at: llvm/include/llvm/Object/ResFile.h:61
+
+  static inline bool classof(const Binary *v) {
+    return v->isRes();
----------------
v -> V


================
Comment at: llvm/include/llvm/Object/ResFile.h:83-84
+  // DWORD align
+  if (Offset % 4 != 0)
+    Offset += 4 - (Offset % 4);
+
----------------
We have `alignTo` function to do this.


================
Comment at: llvm/include/llvm/Object/ResFile.h:87
+  // correct for the fact we already read 2 integers.
+  Offset -= 2*sizeof(uint32_t);
+
----------------
Please fix format.


================
Comment at: llvm/lib/Object/ResFile.cpp:38
+  size_t LeadingSize = sizeof(ResourceMagic) + sizeof(NullEntry);
+  ArrayRef<uint8_t> Ref(reinterpret_cast<const uint8_t*>(Data.getBufferStart() + LeadingSize), Data.getBufferSize() - LeadingSize);
+  return ResEntryRef(Ref, this);
----------------
Format.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:79-81
+  if (!EC)
+    return;
+  reportError(EC.message());
----------------
It's more natural to do `if (EC) reportError(...)`.


https://reviews.llvm.org/D33180