Replace dynamic type dispatch with static type dispatch in strings/numbers.h #62
Conversation
absl/strings/numbers.h
Outdated
| template<typename int_type> | ||
| using IsSignedIntType = | ||
| std::is_signed< | ||
| typename std::conditional_t< |
There was a problem hiding this comment.
conditional_t is a C++14 API. Gotta wait a few more years before we can rely on that in Abseil, I'm afraid.
That said, absl/meta/type_traits.h has an absl::conditional_t.
absl/strings/numbers.h
Outdated
| } | ||
|
|
||
| template<typename any_type> | ||
| struct Identity final |
There was a problem hiding this comment.
This already exists in absl/base/internal/identity.h - maybe it should just move to meta/type_traits? Regardless, for now you should use that one.
absl/strings/numbers.h
Outdated
| Identity<int_type>>::type>; | ||
|
|
||
| template<typename int_type> | ||
| using PrintableIntType = |
There was a problem hiding this comment.
PrintableIntType is a slightly weird name for this - it's printable only in the sense that we've written overloads for it.
Perhaps something more like IntToSupportedInt or IntTo32Or64? I'd definitely name it more actively - it's more metafunction than type.
There was a problem hiding this comment.
Renamed to IntLikeTo32Or64, wrote a comment explaining meaning of this metafunction.
absl/strings/numbers.h
Outdated
| *out = static_cast<int_type>(val); | ||
| } | ||
| } | ||
| using ParsedType = numbers_internal::PrintableIntType<int_type>; |
There was a problem hiding this comment.
ParsedType seems like an odd name: it isn't parsed from anything. SupportedInt?
tituswinters
left a comment
tituswinters
left a comment
There was a problem hiding this comment.
I'll also try to get the original maintainer of this to take a look.
I'm curious: is this not being optimized already? Is this just to address the warnings on MSVC?
|
Thanks for comments. |
|
Update: we're looking at various options for potentially restructuring that to address the MSVC warnings - we might go this way or we might find a different refactoring. I expect it'll get resolved/decided by early next week either way. |
|
The internal maintainer took a different approach, which I think should still resolve the MSVC warnings you were seeing. Check the current HEAD and see if that resolves your issue? If not, we'll finish this off next week. Thanks. |
|
I've looked at current |
…mbers.h a) fixes warnings issued by MSVC (warning C4127: conditional expression is constant), b) removes runtime signedness check for integer-like types.
|
I've changed my code a bit. Maybe this version is ok. |
|
I have one question though. After implementing these changes I have failing tests. Is it intended use of |
|
Since it's internal, I guess that could in theory be relaxed. But I do like the more-explicit code as-is. Here's a thought: can we just #pragma suppress the MSVC warning for that block? That feels potentially less awkward to me, overall. |
|
Hello! Following up on this PR. Vladimir, any thoughts on Titus' suggestion? |
|
One more ping on this PR - any thoughts on Titus' suggestion, @umogSlayer? Are you still seeing MSVC warnings, and would #pragma suppression clear them up? I'm inclined to close this PR by the end of the week if it remains inactive. |
|
Hello! I don't like the idea to disable warnings for blocks of code. It seems to me that such approach is not scalable, if someone in clang or gcc adds the same warning then you'll need to handle that case separately. Maybe it's better to use something like |
|
Apologies for the delay in following up here. We're still leaning towards the pragma solution. The warnings are all false in those cases, and disabling them locally is safe and correct. A SFINAE-based solution feels excessive here. Thoughts? |
|
Given the lack of activity on this PR, I think we should close it--please reopen if you're still seeing MSVC warnings. |
5 participants
a) fixes warnings issued by MSVC (warning C4127: conditional expression is
constant),
b) removes runtime "signedness" check for integer-like types.