Page MenuHomePhabricator

[obj2yaml] - Fix indentations in the ELF output.
Needs ReviewPublic

Authored by grimar on Fri, Feb 14, 6:45 AM.

Details

Summary

This is as about the problem D73045 tried to solve,
but I suggest a completely different approach.

What if we just do a post processing for the output?
I.e. first output it to a string and then using our knowledge
about YAML grammar will reformat indentations.

It separates the formatting pass from the original pass,
what makes the whole approach isolated and simple.

Diff Detail

Event Timeline

grimar created this revision.Fri, Feb 14, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Feb 14, 6:45 AM

Overall, the process seems sound and simple. That probably makes it the right thing to do here. Some comments explaining each step of the post-processing would be useful though. Also, should this really be restricted to ELF?

llvm/test/tools/obj2yaml/elf-output-indentation.yaml
3

an output -> the output

41

Use an arbitrary YAML document
or
Use arbitrary YAML

would be better English. However, I think what you really want to say is something like "Use a YAML document with arbitrary fields and values to show how the indentation is affected."

Also, should this really be restricted to ELF?

I see no obvious reasons for such restrictions. Though I supposed that we might want to start from ELF and then
enable it for other targets. Do you have a preference here? I.e. do you want to see it for all targets from the begining?

Also, should this really be restricted to ELF?

I see no obvious reasons for such restrictions. Though I supposed that we might want to start from ELF and then
enable it for other targets. Do you have a preference here? I.e. do you want to see it for all targets from the begining?

I don't have a strong preference, and am happy for you to go with whatever is easier.

Does this patch handle sequence types with has_FlowTraits (llvm/Support/YAMLTraits.h) set? It is used for auto line wrapping.

Key: [value0, value1, value2,
      value3, value4,
      value5]

I don't know whether obj2yaml has such objects to emit, though.

Maybe you can summarize the problems and ask on llvm-dev?

(I think the direction of D73045 may be fine.)

grimar marked 2 inline comments as done.EditedWed, Feb 19, 12:31 AM

Does this patch handle sequence types with has_FlowTraits (llvm/Support/YAMLTraits.h) set? It is used for auto line wrapping.

Key: [value0, value1, value2,
      value3, value4,
      value5]

Not yet. Yesterday I've found this and other cases where the current code doesn't produce an expected result.
Though it looks like they can be fixed without adding much complexity to the algorithm, but I had no chance to check yet.

I don't know whether obj2yaml has such objects to emit, though.

Maybe you can summarize the problems and ask on llvm-dev?

(I think the direction of D73045 may be fine.)

I do not like the direction of D73045. It looks to me too intrusive for a "add or remove few spaces" task.
(Note that the version posted in D73045 does not solve the problem, it should be extended and it seems
to me that it should be noticably larger to solve the task).

grimar updated this revision to Diff 245650.Thu, Feb 20, 7:02 AM
  • Updated the algorithm used.
  • Added a much better test cases.
  • Added comments.
grimar retitled this revision from [obj2yaml][WIP] - Fix indentations in the ELF output. to [obj2yaml] - Fix indentations in the ELF output..Thu, Feb 20, 7:03 AM
grimar edited the summary of this revision. (Show Details)