-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-44364: [C++] Don't export template class #44365
Conversation
It works with MSVC but doesn't work with clang-cl.
|
I think these were exported precisely because of MSVC at some point, so I'm surprised this isn't needed anymore. |
@github-actions crossbow submit win |
Revision: a365cbf Submitted crossbow builds: ursacomputing/crossbow @ actions-5eb8803ff8 |
I'm not sure when we need https://github.com/apache/arrow/actions/runs/11268303462/job/31334825247#step:9:1229
|
Do we have CI with clang-cl? Or did you test this PR yourself with it? |
No. But we may be able to add it because it seems that we can use it on GitHub Actions: actions/runner-images#10018
@zhanweiw tested: #44310 (comment) But this PR has more |
Well, according to #44310 (comment), only the |
clang-cl CI: #44378 I'll merge this in a few days (for 19.0.0 not 18.0.0) if nobody objects this. |
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit d661607. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
It works with MSVC but doesn't work with clang-cl.
What changes are included in this PR?
Remove
ARROW_EXPORT
from template classes.Are these changes tested?
Yes but with only MSVC. We don't have clang-cl CI jobs.
Are there any user-facing changes?
Yes.