Skip to content

[SandboxIR] Don't add dup instantiation for clang #116387

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsji
Copy link
Member

@jsji jsji commented Nov 15, 2024

On Windows some clang-based compiler(eg: clang-cl, icx) also define _MSC_VER.

On Windows some clang-based compiler(eg: icx) also define _MSC_VER
Copy link
Contributor

@Maetveis Maetveis left a comment

Choose a reason for hiding this comment

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

There is #114308 and #111940 (comment) discussing this same issue ping @fsfod, @bogner. I believe the warning is misleading here, I think we're hitting #14230 (comment).

Taking a deeper look, I believe we would need to add explicit declarations (extern template) with the __declspec(dllexport) attribute in the source file in addition to the (import) declaration in the headers to upgrade just those two methods to being exported from the explicit template class instantiation. I mocked this up on compiler explorer here, but didn't try it with the LLVM build.

@fsfod
Copy link
Contributor

fsfod commented Nov 18, 2024

I'm not sure for which you mean that need to have extern templates for that don't already have them.
For now a quick fix for now could be to change the define to #if defined(_MSC_VER) && defined(LLVM_BUILD_LLVM_DYLIB) so no one doing normal builds should get the warnings. I also realized the explicit function instantiations in source file need to be moved above the class explicit template instantiation definition for clang-cl to actually export them.

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

Successfully merging this pull request may close these issues.

4 participants