Skip to content

Move 'MetadataSections' from 'ImageInspectionElf.h' to SwiftShims #32493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

augusto2112
Copy link
Contributor

This PR moves the MetadataSections definition from ImageInspectionELF to SwiftShims. This is part of the work in unifying the definition of MetadataSections, in order to be used by both ELF and COFF-specific procedures, as well as being acessible from Swift for testing purposes.

@augusto2112 augusto2112 force-pushed the elf-shim-metadata-sections branch from b74b91c to 8c8360b Compare June 22, 2020 19:12
@augusto2112
Copy link
Contributor Author

@compnerd could you start the tests please? :)

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gottesmm - I'm not sure that this is any easier to really follow than the merge and merge in a single step.

@compnerd
Copy link
Member

@swift-ci please test

Copy link
Contributor

@vedantk vedantk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@compnerd
Copy link
Member

I think that this is good to go, and multiple people have reviewed. I still feel like this is unnecessarily splitting up the change, but getting progress is more important than the ideal path. Going to merge this for now so that @augusto2112 can continue making progress. And, thanks @vedantk for helping with this :)

@compnerd compnerd merged commit 79bdd43 into swiftlang:master Jun 29, 2020
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants