[PATCH] D73484: [llvm-readobj] - Improve error message reported by DynRegionInfo.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

All other changes look for to me.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:360
+##    a location and an entity size of the dynamic symbol table from the section header.
+##    The code uses sizeof(Elf_Sym) for an entity size, so it can't be incorrect and
+##    the message printed is a bit shorter.
----------------
grimar wrote:
> jhenderson wrote:
> > Aside: why is it not using the DT_SYMENT dynamic tag?
> I do not know. Perhaps we want to fix this. Also, I think this area is not well tested.
> I can investigate it in a follow-up.
Sounds good.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:139
+  /// Error prefix. Used for error reporting to provide more information.
+  std::string ErrPrefix;
+  /// Region size name. Used for error reporting.
----------------
Let's make this a little more generic. How about something like "InfoContext"?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1877
+        ("section with index " +
+         Twine(DynamicSec - &cantFail(Obj->sections()).front()) + " has ")
+            .str();
----------------
I'd move the " has " to the usage site, since it's always present, and the error message wouldn't make sense without it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73484/new/

https://reviews.llvm.org/D73484