Skip to content
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

xalan-c: Remove use of obsolete unary_ and binary_function for C++17 compatibility #4575

Merged
merged 3 commits into from
Dec 12, 2018
Merged

xalan-c: Remove use of obsolete unary_ and binary_function for C++17 compatibility #4575

merged 3 commits into from
Dec 12, 2018

Conversation

rleigh-codelibre
Copy link
Contributor

See JIRA ticket and original C++17 patch branch which dates back to February with these patches split out for independent re-use.

With these patches applied, it's possible to use xalan-c in C++17 codebases compiled with -std:c++17. Without the patches, std::unary_function and std::binary_function cause compilation to fail, since they were removed with C++17.

@ras0219-msft
Copy link
Contributor

Thanks for the PR!

I'd prefer to avoid committing these large patchfiles to the vcpkg tree if possible -- is there a stable URL that we could download them from instead? I see that the jira ticket has links, but I don't know how durable those will be (Do they get cleared when the bug closes? After a year? Never?).

@rleigh-codelibre
Copy link
Contributor Author

rleigh-codelibre commented Oct 26, 2018

I'm not sure about the JIRA links.

Have you considered an equivalent to Homebrew formula-patches for vcpkg? For example here are the xalan patches. I'll also be putting them here, so could potentially be pulled by vcpkg. But I'm unsure of the lifetime--it may well depend upon how long homebrew requires them before dropping them, which might not align with the vcpkg requirement. And homebrew is often quite aggressive in their cleanups and changes.

If a vcpkg-patches or equivalent existed, I would be happy to put them there. The homebrew-patches repo is nothing but a dumb patch container for pulling patches from.

@rleigh-codelibre
Copy link
Contributor Author

@ras0219-msft I have split out the patches here and then updated this PR to use them. I hope this meets your requirements. It downloads using the commit hash, so will be deterministic if the patches are updated by a future commit. If you wanted to adopt this approach more generally by adopting a vcpkg-patches repository yourself, I'll be happy to transfer the repository to you, or to open a PR to add the patches to your repository.

Regards,
Roger

@rleigh-codelibre
Copy link
Contributor Author

@ras0219-msft I've tested this locally with C++17 builds over the last few weeks using the latest VS2017 and it's all working fine for me. Are the patches in the separate repository for download an acceptable strategy?

@ras0219-msft ras0219-msft self-assigned this Nov 27, 2018
@ras0219-msft
Copy link
Contributor

Yes, this LGTM; thanks for doing that!

We'll definitely consider opening a vcpkg-patches repo in the future, but for now this is a good solution.

Ready to merge pending CI!

@rleigh-codelibre
Copy link
Contributor Author

Awesome, thanks! I'll be happy to migrate these patches over to your patches repo as and when it becomes available.

@ras0219-msft ras0219-msft merged commit e04b4ed into microsoft:master Dec 12, 2018
@ras0219-msft
Copy link
Contributor

Thanks again for the PR!

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.

2 participants