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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/Object/COFF.h:641-644
+  union {
+    support::ulittle32_t DataEntryOffset;
+    support::ulittle32_t SubdirOffset;
+  } Offset;
----------------
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?


================
Comment at: llvm/include/llvm/Object/COFF.h:1076
+    case 1:
+      return StringRef("kRT_CURSOR (ID 1)");
+    case 2:
----------------
ruiu wrote:
> StringRef has a (non-explicit) constructor that takes char*, so you don't need to construct StringRef explicitly. You can just return a string literal.
What about this?


================
Comment at: llvm/include/llvm/Object/COFF.h:1072
+  ErrorOr<const coff_resource_dir_table*> getTableAtOffset(uint32_t Offset);
+  ErrorOr<StringRef> getDirStringAtOffset(uint32_t Offset,) const;
+
----------------
Does it compile? (Look at the stray `,`).


================
Comment at: llvm/include/llvm/Object/COFF.h:1074
+
+  StringRef convertIDToType(uint32_t ID) const {
+    switch (ID) {
----------------
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?)


https://reviews.llvm.org/D32609