Page MenuHomePhabricator

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

Authored by ychen on Jul 12 2019, 3:41 PM.



Introduce getDynamicString to provide better
error handling whenever indexing dynamic string table is needed.

Diff Detail


Event Timeline

ychen created this revision.Jul 12 2019, 3:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 3:41 PM

Do you find another use case of printDynamicString? If yes, can you name it?

300 ↗(On Diff #209624)

Delete = true. The default argument is not used I think.

ychen updated this revision to Diff 209752.Jul 14 2019, 6:34 PM
  • Replace printDynamicString with getDynamicString.
  • Change tests accordingly.
ychen added a comment.Jul 14 2019, 6:36 PM

Do you find another use case of printDynamicString? If yes, can you name it?

Patch updated. I think we could use a function to indexing the dynamic string table.

MaskRay accepted this revision.Jul 14 2019, 6:51 PM
This revision is now accepted and ready to land.Jul 14 2019, 6:51 PM
grimar added inline comments.Jul 15 2019, 12:50 AM
172 ↗(On Diff #209752)

Isn't it inconsistent? For line above you print <error>, but here you print [<error>].
Was it intentional change? (it is not NFC then).

1973 ↗(On Diff #209752)

You do not need else after return:

if (DynamicStringTable.empty())
  return "<String table is empty or was not found>";
if (Value < DynamicStringTable.size())
  return + Value;
ychen retitled this revision from [NFC][llvm-readobj] Refactor dynamic string table indexing into a function. to [llvm-readobj] Refactor dynamic string table indexing into a function..Jul 15 2019, 9:30 AM
ychen edited the summary of this revision. (Show Details)
ychen updated this revision to Diff 209890.Jul 15 2019, 9:32 AM
ychen marked 2 inline comments as done.
  • Address reviewer's comments.
ychen marked an inline comment as done.Jul 15 2019, 9:32 AM
ychen added inline comments.
172 ↗(On Diff #209752)

Title updated to reflect that it is not NFC.

grimar added inline comments.Jul 16 2019, 12:20 AM
172 ↗(On Diff #209752)

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?

MaskRay added inline comments.Jul 16 2019, 12:23 AM
172 ↗(On Diff #209752)

I'm fine with either choice (though I have a slight preference for non-capitalized messages)

grimar added inline comments.Jul 16 2019, 1:01 AM
172 ↗(On Diff #209752)

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: [<..>]

jhenderson added inline comments.Jul 16 2019, 1:56 AM
172 ↗(On Diff #209752)

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.

I am OK with this patch.

172 ↗(On Diff #209752)

I do not really like the new output, but it is not so critical probably.

This revision was automatically updated to reflect the committed changes.