Some code deduplication for plPythonSDLModifier. #1465
Conversation
| if (!hintstring.empty()) | ||
| var->GetNotificationInfo().SetHintString(hintstring); | ||
| return true; | ||
| } else if (PyLong_Check(pyVar)) { |
There was a problem hiding this comment.
Presumably one of those used to be a PyInt_*... I hope >.>
Hoikas
force-pushed
the
pysdl_code_dedupe
branch
2 times, most recently
from
0fe4d52
to
4456bc3
Compare
|
Converting to draft because gcc. |
| } | ||
| return std::nullopt; | ||
| } else { | ||
| static_assert(false, "Unhandled C++ type conversion for Python number"); |
There was a problem hiding this comment.
This is not a dependent expression, and therefore is ill-formed even if the function is never called with the else condition ("no diagnostic required" means that MSVC is allowed to ignore it even though it's invalid, but GCC and Clang do emit a diagnostic)... Some options:
- Leave the base template unimplemented and implement the two (well, three) cases as specializations instead
- Redefine the static_assert to use a dependent condition (something like
static_assert(sizeof(T) != sizeof(T), ...)). Although this is technically still ill-formed, GCC and clang might be happier. - Move the static_assert outside of the
if constexprblock, testing all possible types, and then leave something like astd::unreachable()in the else condition (less DRY).
| return PyLong_AsLong(pyLong.Get()); | ||
| } | ||
| return std::nullopt; | ||
| } else if constexpr (std::disjunction_v<std::is_same<T, float>, std::is_same<T, double>>) { |
There was a problem hiding this comment.
I'm fairly certain you can just
| } else if constexpr (std::disjunction_v<std::is_same<T, float>, std::is_same<T, double>>) { | |
| } else if constexpr (std::is_same_v<T, float> || std::is_same_v<T, double>) { |
There was a problem hiding this comment.
Yeah, that worked with MSVC... I just wasn't sure why gcc and clang were exploding, but you've clarified that nicely :)
| if (!hintstring.empty()) | ||
| var->GetNotificationInfo().SetHintString(hintstring); | ||
| return true; | ||
| } else if (PyLong_Check(pyVar)) { |
There was a problem hiding this comment.
Presumably one of those used to be a PyInt_*... I hope >.>
Hoikas
force-pushed
the
pysdl_code_dedupe
branch
2 times, most recently
from
ff60840
to
5803d54
Compare
There were two branches of the if statement for converting Python longs to C++ longs. Debunk that foolishness. Also, we now accept any object that implements the number protocol instead of just longs and floats. These could even be Python objects that implement the `__int__()` and `__float__()` dunder methods.
Hoikas
force-pushed
the
pysdl_code_dedupe
branch
from
5803d54
to
4c5d799
Compare
|
The most recent push removes an unneeded |
No description provided.