This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj][test] Move tests to binary format specific subdirectories
ClosedPublic

Authored by MaskRay on Nov 14 2019, 11:23 AM.

Details

Summary

Create COFF/, ELF/, and Minidump and move tests there.

Also

  • Rename *.test to *.yaml
  • For yaml2obj RUN lines, use -o %t instead of > %t for consistency. We still have tests that check stdout is the default output, e.g. multi-doc.test
  • Update tests to consistently use ## for comments. # is for RUN and CHECK lines.
  • Merge symboless-relocation.yaml and invalid-symboless-relocation.yaml to ELF/relocation-implicit-symbol-index.test

Event Timeline

MaskRay created this revision.Nov 14 2019, 11:23 AM

Mostly looks fine to me.

llvm/test/tools/yaml2obj/ELF/symbol-index-invalid.yaml
1–2

Maybe add "in a symbol".

llvm/test/tools/yaml2obj/ELF/verdef-section.yaml
2

Delete "the".

llvm/test/tools/yaml2obj/ELF/verneed-section.yaml
2

Delete "the".

llvm/test/tools/yaml2obj/empty-symbols.yaml
10

I might be misreading the intent of the comment, but I think there's value in testing:

Symbols:

distinctly from

Symbols: []

In other words, can yaml2obj handle it with/without the [].

MaskRay updated this revision to Diff 229588.Nov 15 2019, 10:13 AM
MaskRay marked 5 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Address review comments

llvm/test/tools/yaml2obj/empty-symbols.yaml
10

We probably don't have other tests that check Symbols: with the value omitted now.

Added it back with a comment.

grimar accepted this revision.Nov 18 2019, 1:05 AM

Nice cleanup! I've found only a single minor nit.

llvm/test/tools/yaml2obj/ELF/empty-symbols.yaml
10

# -> ##

This revision is now accepted and ready to land.Nov 18 2019, 1:05 AM
jhenderson accepted this revision.Nov 18 2019, 1:54 AM

LGTM, with one suggestion.

llvm/test/tools/yaml2obj/ELF/empty-symbols.yaml
10

This phrasing sounds a bit clunky to me. How about "... but here we show that the value can be omitted."

MaskRay updated this revision to Diff 229864.Nov 18 2019, 9:03 AM
MaskRay marked 2 inline comments as done.

Address review comments

This revision was automatically updated to reflect the committed changes.
llvm/test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml