add std.int128 a 128 bit integer arithmetic type #8426

WalterBright · 2022-04-02T22:35:18Z

A 128 bit signed integer type.

Not done is the unsigned version.

dlang-bot · 2022-04-02T22:35:20Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8426"

CyberShadow · 2022-04-02T22:39:46Z

I think a generic arbitrary-fixed-size integer type would be a better addition.

An implementation exists here:
https://github.com/d-gamedev-team/gfm/blob/master/integers/gfm/integers/wideint.d

CyberShadow · 2022-04-02T22:45:42Z

For fellow under-a-rock-dwellers, this exposes core.int128 via Phobos.

WalterBright · 2022-04-02T22:46:35Z

Blocked by dlang/druntime#3794

CyberShadow · 2022-04-02T22:49:14Z

Is there a link to centralized discussion? My immediate question is that from the Druntime pull requests I got the impression that core.int128 was to be exposed as language-builtin cent/ucent types by the compiler, not via a Phobos module.

ghost · 2022-04-02T23:15:05Z

std/int128.d

+            op == "*" || op == "/" || op == "%" ||
+            op == "&" || op == "|" || op == "^")
+    {
+        if (op == "+")


why no static ifs there ?

WalterBright · 2022-04-02T23:19:41Z

discussion: https://digitalmars.com/d/archives/digitalmars/D/When_will_you_implement_cent_and_ucent_358854.html

Herringway · 2022-04-02T23:31:27Z

Will this come with support for 128-bit literals? Or will we have to wait for importC to gain int128_t support for that?

salihdb

// ++Int128
Int128 opUnary(string op)() const
    if (op == "++")
{
    return Int128(inc(this.data));
}

WalterBright · 2022-04-03T02:38:52Z

Blocked by dlang/druntime#3795

std/int128.d

moon-chilled · 2022-04-02T23:59:41Z

std/int128.d

+    this(long lo)
+    {
+        data.lo = lo;
+        data.hi = lo < 0 ? ~0L : 0;


data.hi = lo >> 63

Also, maybe add a constructor for ulong.

It's not clear what would be faster - I prefer to leave that pattern up to the optimizer!

it's also clearer imo. You're shifting in the high bits

ljmf00 · 2022-04-03T21:31:54Z

std/int128.d

+            op == "*" || op == "/" || op == "%" ||
+            op == "&" || op == "|" || op == "^")
+    {
+        static if (op == "+")


Why we are doing template specialization by hand?

Looks straightforward to me.

For me, looks more readable to find the correct specialization with something like this:

Int128 opBinary(string op : "+")(Int128 op2) const { return Int128(add(this.data, op2.data)); }

Shouldn't be a big deal but, also, specializing in this way would be faster for the compiler to look it up, as opposed to constraints, in which it needs to evaluate the static condition, in this case, twice.

My way is easier to read (for me, anyway) as it emphasizes the commonality.

I'd also prefer the constraint in the header (whether by if or : isn't that important) to avoid duplication, but I allow your style if you prefer it.

dukc

Great addition to Phobos IMO. Two more things to do:

-Documentation unit tests are missing.
-Please add a changelog entry

EDIT: I changed my mind and rejected this PR altogether. See later.

dukc · 2022-04-04T21:52:39Z

std/int128.d

+ */
+module std.int128;
+
+private import core.int128;


Imports are private by default. No need for the keyword.

dukc · 2022-04-04T21:56:28Z

std/int128.d

+    bool opEquals(Int128 op2) const
+    {
+        return data.hi == op2.data.hi && data.lo == op2.data.lo;
+    }


Does leaving this function off result in errors? I'd think the default struct comparison would work.

dukc · 2022-04-04T21:59:37Z

std/int128.d

+        return data.hi == op2.data.hi && data.lo == op2.data.lo;
+    }
+
+    // +Int128


Please, leave comments like this off. If the reader is so bad at D that he does not understand the function signature then he's beyond help anyway.

dukc · 2022-04-04T22:00:36Z

std/int128.d

+}
+}
+
+unittest


Add attributes.

see line 324

I don't want to block it

maxhaton · 2022-04-04T23:50:59Z

I don't see why this should be in Phobos versus druntime. Having 128 bit types as a library is already bizarre compared to what you get in basically any other systems programming language (even if we then special case them to use the proper compiler intrinsics i.e. the code generated at the moment is terrible), but if we really do that why would they be split across phobos and druntime?

ljmf00 · 2022-04-05T01:43:06Z

I don't see why this should be in Phobos versus druntime. Having 128 bit types as a library is already bizarre compared to what you get in basically any other systems programming language (even if we then special case them to use the proper compiler intrinsics i.e. the code generated at the moment is terrible), but if we really do that why would they be split across phobos and druntime?

Thinking about that, I also think we should define a decent public interface on druntime and not duplicate it on phobos. We do a similar duplication for, e.g. std.math and core.math builtins/intrinsics, which is currently horrible. See https://sourcegraph.com/github.com/dlang/dmd/-/blob/src/dmd/toir.d?L523 .

Another thing I'm currently thinking about wrapping Cent into a different aligned struct: Isn't this going to make loads/stores of the struct data potentially slower? Why not provide those interfaces on the aligned struct instead of wrapping it?

WalterBright · 2022-04-05T07:41:24Z

Having 128 bit types as a library is already bizarre

Not really. See std.complex. It used to be a builtin type, but D got to the point where it could be done handily as a library type. Int128 is a rarely used type (yes, I know some people use it a lot). I've never had a use for it in 45 years of programming. So why make the compiler more complex than it needs to be?

Having a 128 bit integer will bloat a lot of the internal data structures, that are used a lot, by 64 bits. That's pretty expensive.

why would they be split across phobos and druntime?

Druntime provides the guts that need to be adjusted for each target. Phobos provides the user interface. std.complex is in Phobos. We also split math routines between druntime and phobos.

Other languages do it as a builtin type because their metaprogramming facilities are deficient or nonexistent.

Lastly, doing it in Phobos means it can be done now.

WalterBright · 2022-04-13T05:02:40Z

Style sez:

std/int128.d(47:15)[warn]: Public declaration 'Int128' has no documented example.

but it obviously does.

ibuclaw · 2022-04-13T07:06:59Z

Style sez:

std/int128.d(47:15)[warn]: Public declaration 'Int128' has no documented example.

but it obviously does.

Hazarding a guess, but wouldn't that mean documented unittest?

i.e

/// Documented declaration
struct Foo
{
}

/// Documented example
unittest
{
    Foo x;
}

WalterBright · 2022-04-14T05:15:02Z

@ibuclaw Same error. I don't know what it wants.

WalterBright · 2022-04-14T05:24:12Z

@RazvanN7 do you know what DScanner wants here?

RazvanN7 · 2022-04-14T06:45:59Z

@WalterBright :

std/int128.d(19:15)[warn]: Public declaration 'Int128' has no documented example.

RazvanN7 · 2022-04-14T06:57:47Z

@WalterBright The version(unittest) might confuse D-scanner. You could try putting the documented unittest right after the Int128 declaration.

You can run the check locally:

make -f posix.mak style

WalterBright · 2022-04-14T07:40:20Z

I tried it:

embedded in the documentation for the struct
as a member of the struct
following the closing brace of the struct

No joy. I filed an issue for dscanner:

dlang-community/D-Scanner#862

In the meantime, how do I disable this check?

RazvanN7 · 2022-04-14T07:43:40Z

There's a .dscanner.ini file that configures which checks are enabled for each file. There is a undocumented_declaration_check variable which currently lists the files for which the check is not enabled. You can add -std.int128 to that list of checks.

WalterBright · 2022-04-14T07:45:28Z

Thank you. Also, dscanner does not come with the distribution.

Oh, I see why I didn't see the .dscanner.ini earlier. It has the . which hides it. What is the purpose of hiding critical files?

RazvanN7 · 2022-04-14T07:48:37Z

What is the purpose of hiding critical files?

I have no idea. When I started contributing to D this modus operandi was already put in place, but I would assume that, ideally, all checks would be enabled for all files, so it might be that the config file is just a stop gap solution until we reach that point.

WalterBright · 2022-04-14T07:48:44Z

.dscanner.ini also explains why doing it the way std.complex didn't work, either. That's in the disable list.

WalterBright · 2022-04-14T07:51:14Z

It's good that dmd.conf is not .dmd.conf

dkorpel · 2022-04-14T10:20:41Z

std/int128.d

+
+    Cent data;          /// core.int128.Cent
+
+    /****************


Use the right style in documentation comments

https://dlang.org/dstyle.html#phobos_documentation

I used the correct style. Dscanner does not work.

This is not about Dscanner, and you're not using the correct style. There's extra stars.

Documentation comments should not use more than two stars /** or two pluses /++ in the header line.
(...)

Documentation comments should not have leading stars on each line.

https://dlang.org/dstyle.html#phobos_documentation

WalterBright · 2022-04-14T17:59:22Z

There is a undocumented_declaration_check variable which currently lists the files for which the check is not enabled. You can add -std.int128 to that list of checks.

Did that. It still fails that check.

I tried your suggestion. It does not work.

ljmf00 · 2022-04-16T14:11:43Z

So what about the cent/ucent in the spec and ABI? They are useless now?

WalterBright · 2022-04-18T01:13:42Z

We may eventually implement cent and ucent. This, in the meantime, will work.

Fix Issue: #20139 add std.int128 a 128 bit integer arithmetic type

WalterBright added the Enhancement label Apr 2, 2022

WalterBright requested review from andralex, CyberShadow, MartinNowak and wilzbach as code owners April 2, 2022 22:35

WalterBright mentioned this pull request Apr 2, 2022

copy core/int128.d to import directory dlang/druntime#3794

Merged

WalterBright added the Blocked label Apr 2, 2022

ghost reviewed Apr 2, 2022

View reviewed changes

WalterBright force-pushed the int128 branch 2 times, most recently from 2691839 to 11c1ad6 Compare April 3, 2022 00:48

salihdb reviewed Apr 3, 2022

View reviewed changes

WalterBright mentioned this pull request Apr 3, 2022

relax alignment of Cent for 32 bit targets dlang/druntime#3795

Merged

WalterBright force-pushed the int128 branch from 11c1ad6 to ee79fa7 Compare April 3, 2022 02:55

moon-chilled reviewed Apr 3, 2022

View reviewed changes

ljmf00 previously requested changes Apr 3, 2022

View reviewed changes

WalterBright force-pushed the int128 branch from ee79fa7 to 326fca1 Compare April 3, 2022 21:41

WalterBright changed the title ~~add std.int128~~ add std.int128 a 128 bit integer arithmetic type Apr 4, 2022

dukc suggested changes Apr 4, 2022

View reviewed changes

WalterBright force-pushed the int128 branch from dbb8802 to cd89350 Compare April 14, 2022 04:16

WalterBright force-pushed the int128 branch 2 times, most recently from fd8e402 to d113d61 Compare April 14, 2022 06:21

WalterBright mentioned this pull request Apr 14, 2022

std/int128.d(19:15)[warn]: Public declaration 'Int128' has no documented example dlang-community/D-Scanner#862

Open

WalterBright force-pushed the int128 branch from d113d61 to 5140d8a Compare April 14, 2022 07:49

dkorpel previously requested changes Apr 14, 2022

View reviewed changes

add std.int128

60f5bfa

WalterBright force-pushed the int128 branch from 5140d8a to 60f5bfa Compare April 14, 2022 18:01

WalterBright removed the Blocked label Apr 14, 2022

RazvanN7 approved these changes Apr 15, 2022

View reviewed changes

RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 15, 2022

RazvanN7 merged commit dc6fcbd into dlang:master Apr 18, 2022

iK4tsu pushed a commit to iK4tsu/phobos that referenced this pull request Apr 18, 2022

Merge pull request dlang#8426 from WalterBright/int128

b2a0c6c

Fix Issue: #20139 add std.int128 a 128 bit integer arithmetic type

WalterBright deleted the int128 branch April 18, 2022 19:27

add std.int128 a 128 bit integer arithmetic type #8426

add std.int128 a 128 bit integer arithmetic type #8426

Conversation

WalterBright commented Apr 2, 2022

dlang-bot commented Apr 2, 2022

Bugzilla references

Testing this PR locally

CyberShadow commented Apr 2, 2022

CyberShadow commented Apr 2, 2022

WalterBright commented Apr 2, 2022

CyberShadow commented Apr 2, 2022

ghost Apr 2, 2022 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright commented Apr 2, 2022

Herringway commented Apr 2, 2022

salihdb left a comment

Choose a reason for hiding this comment

WalterBright commented Apr 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukc left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxhaton commented Apr 4, 2022

ljmf00 commented Apr 5, 2022

WalterBright commented Apr 5, 2022

WalterBright commented Apr 13, 2022

ibuclaw commented Apr 13, 2022

WalterBright commented Apr 14, 2022

WalterBright commented Apr 14, 2022

RazvanN7 commented Apr 14, 2022 • edited

RazvanN7 commented Apr 14, 2022 • edited

WalterBright commented Apr 14, 2022

RazvanN7 commented Apr 14, 2022

WalterBright commented Apr 14, 2022

RazvanN7 commented Apr 14, 2022

WalterBright commented Apr 14, 2022

WalterBright commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright commented Apr 14, 2022

ljmf00 commented Apr 16, 2022

WalterBright commented Apr 18, 2022

ghost Apr 2, 2022 •

edited by ghost

dukc left a comment •

edited

RazvanN7 commented Apr 14, 2022 •

edited

RazvanN7 commented Apr 14, 2022 •

edited