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

more operator of int128 #3559

Closed
wants to merge 36 commits into from
Closed

more operator of int128 #3559

wants to merge 36 commits into from

Conversation

steve02081504
Copy link

No description provided.

@steve02081504 steve02081504 requested a review from a team as a code owner March 11, 2023 03:50
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many operators are not needed.

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can further overhaul the conversion functions. It might be better to add test coverage to tests/std/tests/P1522R1_difference_type/test.cpp.

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
steve02081504 and others added 2 commits March 11, 2023 14:59
@steve02081504
Copy link
Author

steve02081504 commented Mar 11, 2023

@fvs[@frederick-vs-ja]

int a[3];
_Signed128 b=2;
a[b]=5;//error

I'm not quite sure how to fix this, it doesn't look like it's something that can be done with custom types.....

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
steve02081504 and others added 3 commits March 11, 2023 15:15
Co-authored-by: Igor Zhukov <fsb4000@yandex.ru>
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
Co-authored-by: Igor Zhukov <fsb4000@yandex.ru>
@fsb4000
Copy link
Contributor

fsb4000 commented Mar 11, 2023

Could you run clang-format -i stl/inc/xtr1common?

diff --git a/stl/inc/xtr1common b/stl/inc/xtr1common
index 35ddaf0d..d0b14e8a 100644
--- a/stl/inc/xtr1common
+++ b/stl/inc/xtr1common
@@ -179,14 +179,13 @@ struct _Signed128;
 struct _Unsigned128;
 
 _EXPORT_STD template <class _Ty>
-_INLINE_VAR constexpr bool is_integral_v = _Is_any_of_v<remove_cv_t<_Ty>, bool, char, signed char, unsigned char,
-    wchar_t,
+_INLINE_VAR constexpr bool is_integral_v =
+    _Is_any_of_v<remove_cv_t<_Ty>, bool, char, signed char, unsigned char, wchar_t,
 #ifdef __cpp_char8_t
-    char8_t,
+        char8_t,
 #endif // __cpp_char8_t
-    char16_t, char32_t, short, unsigned short, int, unsigned int, long, unsigned long, long long, unsigned long long,
-    _Signed128, _Unsigned128
->;
+        char16_t, char32_t, short, unsigned short, int, unsigned int, long, unsigned long, long long,
+        unsigned long long, _Signed128, _Unsigned128>;
 
 _EXPORT_STD template <class _Ty>
 struct is_integral : bool_constant<is_integral_v<_Ty>> {};

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There're still some problems.

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/xtr1common Outdated Show resolved Hide resolved
@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Mar 11, 2023

@fvs[@frederick-vs-ja]

int a[3];
_Signed128 b=2;
a[b]=5;//error

I'm not quite sure how to fix this, it doesn't look like it's something that can be done with custom types.....

It's impossible to "fix" this, because the built-in operator[] only accepts integer and unscoped enumeration types for index operands.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Mar 11, 2023
steve02081504 and others added 2 commits March 12, 2023 12:54
thx for help

Co-authored-by: Igor Zhukov <fsb4000@yandex.ru>
@strega-nil-ms
Copy link
Contributor

Is there a need for these operators? _Int128 is intentionally an internal type which we do not want people using, except as a difference_type, so it doesn't need to be a fully integer type.

@fsb4000
Copy link
Contributor

fsb4000 commented Mar 13, 2023

@strega-nil-ms , no it's not an internal type.

People want int128: https://developercommunity.visualstudio.com/t/Support-for-128-bit-integer-type/879048

I added a workaround there:

#include <ranges>

using int128 = std::ranges::range_difference_t<std::ranges::iota_view<long long, long long>>;

and later @frederick-vs-ja backported the library solution to C++14 (i.e., to all supported modes) (#3036) and added a workaround there:

users may include <__msvc_int128.hpp>, and obtain 128-bit integer-class types std::_Signed128 and std::_Unsigned128 in all modes

@steve02081504
Copy link
Author

tbh, I don't really care if _Int128 should be allowed for users or not.
I started out just wanting to make my project's intmax type bigger if possible, so https://github.com/ELC-lang/ELC/blob/6742483e8d347acd623c44b9d548817ebd909d98/parts/_share/basic_environment/_body.hpp#L41L45
Then I found some problems compiling it, so I fixed it along the way and submitted this pr, and that's all I did.
I didn't even think it would take this long to pr for a big project, it's probably the first time I've done something like this?

@barcharcraz
Copy link
Member

barcharcraz commented Mar 14, 2023

even if we did want to support 128bit extended types I'm not sure documenting _Int128 would really satisfy that feature request, esp since it probably wouldn't be possible to make it a drop-in replacement for the GCC/Clang functionality.

@steve02081504 steve02081504 requested review from frederick-vs-ja and fsb4000 and removed request for frederick-vs-ja and fsb4000 March 14, 2023 10:36
@StephanTLavavej
Copy link
Member

Thanks for looking into this. We talked about it at the weekly maintainer meeting and have decided to close this PR without merging, because this is implementing a non-Standard extension, which is explicitly a non-goal of our project as mentioned in our README.

Specifically, the requirements for integer-class types are listed in WG21-N4928 [iterator.concept.winc], and (despite the name of the type category) they don't require "behaves like real integer types in all conceivable ways". In particular, conversions to and from floating-point types aren't required at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants