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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/Object/Binary.h:60
 
+    ID_RES,
+
----------------
ruiu wrote:
> 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.
Maybe `ID_WinRes,  // Windows resource data`


================
Comment at: llvm/include/llvm/Object/ResFile.h:10
+//
+// This file declares the .res file class.
+//
----------------
ruiu wrote:
> the Windows .res, as Unix developers usually don't know what .res is.
Also maybe a brief sentence about what a .res file is.


================
Comment at: llvm/include/llvm/Object/ResFile.h:28-30
+#define RETURN_IF_ERROR(X)                                                     \
+  if (auto EC = errorToErrorCode(X))                                           \
+    return EC;
----------------
ruiu wrote:
> 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.)
Alternatively, move the macro to a cpp file (see below for related comment), or move the macro to the same place where `errorToErrorCode` is defined, so other people can make use of it and we don't have to copy the code around anymore.


================
Comment at: llvm/include/llvm/Object/ResFile.h:32-34
+static const char ResourceMagic[] = {
+    '\0',   '\0',   '\0', '\0', '\x20', '\0',   '\0', '\0',
+    '\xff', '\xff', '\0', '\0', '\xff', '\xff', '\0', '\0'};
----------------
This can probably go to the cpp file if you de-inline the functions below.


================
Comment at: llvm/include/llvm/Object/ResFile.h:36
+
+static const char NullEntry[16] = {'\0'};
+
----------------
Same.


================
Comment at: llvm/include/llvm/Object/ResFile.h:68
+private:
+  friend class ResEntryRef;
+};
----------------
I think you should store the `BinaryByteStream` here in this class, not in the `Ref` class.  This makes a lot of things much easier.  See below for an example.


================
Comment at: llvm/include/llvm/Object/ResFile.h:75
+
+inline std::error_code ResEntryRef::moveNext(bool &End) {
+  uint32_t DataSize;
----------------
None of these seem like good candidates for inline functions.  As an aside, if we did want to inline them, the best thing to do is declare them in the body of the class, not after you have finished definiing the class.  In any case, can you move these to a cpp file?


================
Comment at: llvm/include/llvm/Object/ResFile.h:82-87
+  // DWORD align
+  if (Offset % 4 != 0)
+    Offset += 4 - (Offset % 4);
+
+  // correct for the fact we already read 2 integers.
+  Offset -= 2*sizeof(uint32_t);
----------------
Ideally we'd like to get rid of as much math as possible.  `BinaryStreamWriter` has a function called `padToAlign` that does exactly this.  Perhaps you could add `alignTo(unsigned N)` to `BinaryStreamReader which simply skips the appropriate number of bytes.


================
Comment at: llvm/lib/Object/ResFile.cpp:29-32
+	std::unique_ptr<ResFile> Ret(new ResFile(Source, Err));
+	if (Err)
+    return std::move(Err);
+  return std::move(Ret);
----------------
Since this code is already part of the `ResFile` class, I don't know if passing the `Error` as an output parameter is necessary.  Just move the constructor logic to this function, and make the constructor (declared private), take final values that require no processing.


================
Comment at: llvm/lib/Object/ResFile.cpp:37-38
+ResEntryRef ResFile::HeadEntry() {
+  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);
----------------
If the `BBS` is in the `ResFile` instead of the `ResFileRef`, and the `ResFileRef` instead stores a `BinaryStreamRef`, you can just write:

`return ResEntryRef(BBS.slice(LeadingSize, BBS.size()-LeadingSize));`


https://reviews.llvm.org/D33180