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 missing export symbol and suppress dll-interface warnings for MSVC #221

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

panosk
Copy link
Contributor

@panosk panosk commented Jan 14, 2021

MSVC was complaining about this free function and wouldn't link the library with the clients.

There were also countless dll-interface warnings which were polluting the compilation output. See also here:
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4251?view=msvc-160
My understanding is that all these messages are related to STL-derived types in various places.

@guillaumekln
Copy link
Collaborator

Thanks! Could you rebase the branch on master?

@panosk panosk force-pushed the add-missing-export-symbol branch from 5fd5000 to d41f035 Compare January 14, 2021 09:05
@panosk panosk closed this Jan 14, 2021
@panosk panosk force-pushed the add-missing-export-symbol branch from d41f035 to 9ba8132 Compare January 14, 2021 09:09
@panosk
Copy link
Contributor Author

panosk commented Jan 14, 2021

Hm, I messed up rebasing. Can you help, or should I just ditch this PR and send a new one?

@guillaumekln
Copy link
Collaborator

Can you try cherry-picking the commit onto your branch?

git checkout add-missing-export-symbol
git cherry-pick d41f03512b1f0692f49fdb0e881828629c4a3e61

@panosk
Copy link
Contributor Author

panosk commented Jan 14, 2021

Thanks! Will try a bit later once I get back my Win laptop where I've made the changes. The mess probably happened because I created this branch from yesterday's branch fix-sp-flags-for-msvc which I deleted from my fork, and not from my rebased master...

@panosk panosk reopened this Jan 14, 2021
@panosk
Copy link
Contributor Author

panosk commented Jan 14, 2021

Success, thank you!

@guillaumekln
Copy link
Collaborator

Thanks. I will look to add a Windows job in the CI to avoid these errors in the future.

@guillaumekln guillaumekln merged commit a7a2f74 into OpenNMT:master Jan 14, 2021
@panosk panosk deleted the add-missing-export-symbol branch September 20, 2021 17:44
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