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

Fix generic_strtod for clang-cl #156

Closed
wants to merge 1 commit into from
Closed

Conversation

marstaik
Copy link

Fixed to use explicit template specialization that call the function
instead of being a function pointer. The end result is the exact same.

Fixes #136

Fixed to use explicit template specializationg that call the function
instead of being a function pointer. The end result is the exact same.
@marstaik
Copy link
Author

I'm not entirely sure about constexpr function pointers, apparently it is valid because the linker should be taking care of it, but its failing in clang-cl, and it may just be because clang-cl has a bug.

This change however fixes the issue without introducing any harmful side effects, its just a difference of how the template is implemented.

@skrobinson
Copy link
Contributor

skrobinson commented Jan 25, 2022

I oppose merging this change at this time. I'd prefer to leave this PR open so any others with this error have a possible fix and can upvote your solution.

argparse works with MSVC and with Clang. The combination of these toolsets exhibits the error. Sounds like a clang-cl bug to me.

But, merging or not is a decision for @p-ranav.

@felipegodias
Copy link

Do we have an ETA regarding on this? Without the fix builds are still failing for clang-cl version 13.0.1. Works fine for MSVC, GCC, Clang and AppleClang tho.

@skrobinson
Copy link
Contributor

My thinking is that the build errors you and @marstaik are seeing are clang-cl bugs, not argparse bugs. Have you tried submitting an Issue with clang-cl?

But maybe I'm wrong and we should fix this in argparse. Can you test two other solutions?

A) Use static storage for the function. I hope this provides a function address early enough for constexpr assignment.

- template <class T> constexpr auto generic_strtod = nullptr;
+ template <class T> static constexpr auto generic_strtod = nullptr;

B) Replace constexpr with inline const. This should retain many of the benefits of constexpr, while not requiring constexpr versions of strto{d,f,ld}.

- template <class T> constexpr auto generic_strtod = nullptr;
- template <> constexpr auto generic_strtod<float> = strtof;
- template <> constexpr auto generic_strtod<double> = strtod;
- template <> constexpr auto generic_strtod<long double> = strtold;
+ template <class T> inline const auto generic_strtod = nullptr;
+ template <> inline const auto generic_strtod<float> = strtof;
+ template <> inline const auto generic_strtod<double> = strtod;
+ template <> inline const auto generic_strtod<long double> = strtold;

Let me know if either of these work for clang-cl.

@felipegodias
Copy link

@skrobinson the inline solution worked on all toolchains (MSVC, ClangCL, GCC, Clang and AppleClang)

@p-ranav p-ranav closed this in a8e2823 Aug 13, 2022
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.

Project using argparse won't compile using clang-cl
3 participants