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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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>]
----------------
grimar wrote:
> 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: `[<..>]`
My thought is that `[<...>]` looks a bit weird, especially when it's inconsistent. That being said, the two are significantly different parts of the output - one is in the header blurb at the start of the output, the other is in the dynamic table output, so actually I'm not sure consistency here is such an issue.

In dynamic table printing, everything is surrounded by square brackets for good output. Not having the square brackets could be confusing for consumers when they hit bad output, I guess.

I think I could make an argument for it either way around, and I don't think either stands out more than the other. Therefore, I think we should just go with what is easiest in the code. In this case, I think that's the current proposed patch, since it avoids having something come back from getDynamicString saying whether to add the square brackets or not. I don't really mind though, if a good alternative implementation can be found for the other approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64674