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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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.
----------------
grimar wrote:
> jhenderson wrote:
> > Let's make this a little more generic. How about something like "InfoContext"?
> Ok. Seems leaving the "Error prefix" sentence for the comment is fine?
Yes, I think so. The comment might rot if it starts getting used for other things, but I don't think it matters too much in this case.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1877
+        ("section with index " +
+         Twine(DynamicSec - &cantFail(Obj->sections()).front()) + " has ")
+            .str();
----------------
grimar wrote:
> jhenderson wrote:
> > I'd move the " has " to the usage site, since it's always present, and the error message wouldn't make sense without it.
> `ErrPrefix` is often empty. E.g. messages like:
> `invalid DT_RELASZ value (24) or DT_RELAENT value (255)` does not suppose to have this prefix about section index.
> 
> Do you mean to add "has" to the usage site when `ErrPrefix` is not empty?
> 
> 
Oh, I missed that. I'd probably do as you suggest - add "has" when the prefix is non-empty.


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

https://reviews.llvm.org/D73484