[PATCH] D32609: Update llvm-readobj -coff-resources to display tree structure.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/include/llvm/Object/COFF.h:641-644
+  union {
+    support::ulittle32_t DataEntryOffset;
+    support::ulittle32_t SubdirOffset;
+  } Offset;
----------------
ruiu wrote:
> ecbeckmann wrote:
> > ruiu wrote:
> > > You are not using this class, are you? If you plan to use it later, please introduce this later.
> > It is used in COFFDumper when we display each entry in a table.
> So I do not see why you need this. Even if you need this, do you really need this many fields? They are actually pointing to the same memory location that is at offset 0 of a struct, so this is equivalent to
> 
>   union coff_resource_dir_entry {
>     support::ulittle32_t NameOffset;
>     support::ulittle32_t ID;
>     support::ulittle32_t DataEntryOffset
>     support::ulittle32_t SubdirOffset;
>   };
> 
> It seems to me that you don't need to add this struct. You can just handle resource directory entry as a ulittle32_t, can't you?
Hmm I'm not quite sure what you mean.  A resource directory entry is composed of two separate 32-bit fields, 64 bits in total.  The first field can be either an offset to a string name or an ID number.  The second field is either an offset to the resource data or the offset to the next directory level down the tree.  We must access both of these fields to read an entry, we might be able to use a ulittle32_t to read the first field and then move up 4 bytes to read the second field, but it seems much clearer to be able to access either of them by name.


================
Comment at: llvm/include/llvm/Object/COFF.h:1074
+
+  StringRef convertIDToType(uint32_t ID) const {
+    switch (ID) {
----------------
ruiu wrote:
> As a local convention of this directory, it seems functions that returns a name of something is named getSomething. So this should be named getIDName. (But what ID is this?)
This should actually be a function that is part of the coff_resource_dir_entry structure.


https://reviews.llvm.org/D32609