argparse icon indicating copy to clipboard operation
argparse copied to clipboard

Fix generic_strtod for clang-cl

Open marstaik opened this issue 3 years ago • 5 comments

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

Fixes https://github.com/p-ranav/argparse/issues/136

marstaik avatar Jan 21 '22 21:01 marstaik

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.

marstaik avatar Jan 21 '22 21:01 marstaik

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.

skrobinson avatar Jan 25 '22 20:01 skrobinson

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.

felipegodias avatar Aug 04 '22 00:08 felipegodias

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.

skrobinson avatar Aug 04 '22 16:08 skrobinson

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

felipegodias avatar Aug 04 '22 16:08 felipegodias