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

add fix for GCC. #40

Closed
wants to merge 4 commits into from
Closed

add fix for GCC. #40

wants to merge 4 commits into from

Conversation

erip
Copy link
Contributor

@erip erip commented Sep 25, 2020

Motivation

Why are you making this change?

To fix build with GCC so PyTorch can build with newest commits.

Summary

Updates return type to be explicitly pinned within the class. SFINAE etc. etc.

How does the code work?

Why did you choose this approach?

https://stackoverflow.com/a/14419935/2883245

Test Plan

How did you test this change?

Any change that adds functionality should add a unit test as well.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 25, 2020
@erip
Copy link
Contributor Author

erip commented Sep 28, 2020

cc @dreiss @passy Any chance of review/merge for this one? I've got PyTorch Java bindings building here with my fork of fbjni which incorporates these changes and tests pass with both compilers so hopefully not too controversial.

@dreiss
Copy link
Contributor

dreiss commented Sep 28, 2020

I'm probably going to need a few days, unfortunately. Do you happen to know which version of GCC requires this?

@erip
Copy link
Contributor Author

erip commented Sep 28, 2020

I think it's gcc 7.x - I can double check. I guess I misremembered - I think it was gcc 5.4. See godbolt.

@erip
Copy link
Contributor Author

erip commented Oct 1, 2020

@dreiss - any updates on this? If there are things I can help with, I'm happy to.

@erip
Copy link
Contributor Author

erip commented Oct 6, 2020

@dreiss Will you have any time to look at this in the next day or so? I was hoping to see Java bindings for Windows land in PyTorch 1.7.0, but this is now not possible since the 1.7.0 RC has already been cut. I'd still like to see it land ASAP and this is the last thing standing in the way. Since this is such a small PR, I'm hoping it can be imported into phabricator and land cleanly.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@dreiss has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cxx/fbjni/detail/Meta.h Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@erip has updated the pull request. You must reimport the pull request before landing.

@erip
Copy link
Contributor Author

erip commented Oct 13, 2020

@dreiss thanks for making the required changes. Is there a reason my contributions are being committed directly to the repo instead of being merged? This is the second PR that's been ignored with nearly identical fixes being added directly to the repo.

@erip erip closed this Oct 16, 2020
@erip erip reopened this Oct 16, 2020
@erip erip closed this Oct 16, 2020
@erip erip deleted the feature/fix-gcc branch October 16, 2020 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants