[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/lib/Object/WindowsResource.cpp:64-71
+  uint32_t Offset = DataSize + HeaderSize;
+
+  // Correct for the fact we already read 2 integers.
+  Offset -= 2 * sizeof(uint32_t);
+
+  // Advance and align.
+  RETURN_IF_ERROR(Reader.skip(Offset));
----------------
zturner wrote:
> ecbeckmann wrote:
> > zturner wrote:
> > > zturner wrote:
> > > > Maybe I'm missing something, but I don't think any of these lines are needed?  You're computing `Offset` here by adding together the sizes of the two fields you just wrote.  Then subtracting the size of the two fields you just wrote, at which point `Offset` is going to be 0.  So you're skipping zero bytes, then padding to 4 byte alignment, but since you just read 2 uint32s, it should already be 4 byte aligned.
> > > > 
> > > > I think you can just delete all of this?
> > > Ahh I think I follow now.  But why are you just reading the data and header size, but not the *actual* data or headers?
> > This patch was just to add functionality for iterating through the entries without reading them just yet.  It will be easy in future patches to add functions to ResourceEntryRef to return specific fields from each entry.
> I see.  Can you then change to this?
> 
> ```
> BinaryStreamRef HeaderBytes;
> BinaryStreamRef DataBytes;
> RETURN_IF_ERROR(Reader.readStreamRef(HeaderBytes, HeaderSize));
> RETURN_IF_ERROR(Reader.readStreamRef(DataBytes, DataSize));
> RETURN_IF_ERROR(Reader.padToAlignment(sizeof(uint32_t));
> ```
> 
> BTW, it seems strange to me that data size comes first and header size comes second.  Are you sure the first two reads aren't reversed?
> 
> At least this way the logic is more clear and there's no math or manual jumping around which makes it hard to follow.
Ah I see.  In this case HeaderBytes and DataBytes should be made class variables as well.

I agree that it is a strange convention, and indeed the actual Microsoft documentation says that the order is Header Size followed by Data Size.  However, this documentation is incorrect, and I've verified that the order is in fact Data Size > Header Size by examining several .res files in the hexcode editor.


https://reviews.llvm.org/D33180