[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/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));
----------------
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.


https://reviews.llvm.org/D33180