[PATCH] D64674: [llvm-readobj] Refactor dynamic string table indexing into a function.

Tue Jul 16 01:02:29 PDT 2019

grimar added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test:172
+# BAD-STRTAB-LLVM: LoadName: <String table is empty or was not found>
+# BAD-STRTAB-LLVM: 0x0000000000000001  NEEDED   Shared library: [<String table is empty or was not found>]
+# BAD-STRTAB-GNU:  0x0000000000000001 (NEEDED)  Shared library: [<String table is empty or was not found>]
----------------
MaskRay wrote:
> grimar wrote:
> > ychen wrote:
> > > grimar wrote:
> > > > Isn't it inconsistent? For line above you print `<error>`, but here you print `[<error>]`.
> > > > Was it intentional change? (it is not NFC then).
> > > Title updated to reflect that it is not NFC.
> > So, previously we printed `[valid string]` and `<error text>`.
> > After this patch can print `[valid string]`, `[<error text>]` and `<error text>`. 
> > What is not very consistent. Why the new behavior is desired?
> > 
> > @jhenderson, what do you think?
> I'm fine with either choice (though I have a slight preference for non-capitalized messages)
I am talking about the types of brackets used, not about capitalization. I.e. before this patch we had 2 forms: `[..]`, `<..>` and it introduced a third one: `[<..>]`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64674