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

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/include/llvm/Object/ResFile.h:28-30
+#define RETURN_IF_ERROR(X)                                                     \
+  if (auto EC = errorToErrorCode(X))                                           \
+    return EC;
----------------
zturner wrote:
> 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.
Moved to the cpp file.  I'm not sure if we want to put into Error.h because there are some variations in the codebase between returning errors and error codes.


================
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);
----------------
zturner wrote:
> zturner wrote:
> > zturner wrote:
> > > 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));`
> > Alternatively, `return ResEntryRef(BBS.drop_front(LeadingSize).drop_back(LeadingSize));`
> My bad, I misunderstood this code the first time.  It's just `return ResEntryRef(BBS.drop_front(LeadingSize));`
I opted to just have ResourceEntryRef take a BinaryStreamRef instead of an ArrayRef.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:79-81
+  if (!EC)
+    return;
+  reportError(EC.message());
----------------
ruiu wrote:
> It's more natural to do `if (EC) reportError(...)`.
true....but llvm style guide prefers early return


https://reviews.llvm.org/D33180