(This is a follow-up from the other thread:  https://forum.dlang.org/thread/rdrqedmbknwrppbfixll@forum.dlang.org)


I start this new thread to discuss and improve the usage of std.experimental.checkedint
 as a drop-in replacement of native long.

https://dlang.org/phobos/std_experimental_checkedint.html


`Checked` is a struct, try to use it as a `long` require some substantial change of existing code that previously use native long.

I list 8 things I've found below. Suggestions or PR welcome, e.g. even on how to improve the current implementation of `struct Checked`, or help from the compiler.

Thanks.


https://github.com/mingwugmail/dlang_tour/blob/master/divbug.d#L38
------------------------------------------------------------------------
alias Long = Checked!long;


void use_checked() {
  long la, lb, lc;  // native
  Long ca, cb, cc;  // checkedint

  // 0. as array literal
  long[] larr = [1, 2, 3];
//Long[] carr = [1, 2, 3];  // Error: cannot implicitly convert expression [1, 2, 3] of type int[] to Checked!(long, Abort)[]
  Long[] carr = [checked(1L), checked(2L), checked(3L)];  // more verbose

  // 1. checkedint cannot be chain assigned
  la = lb = lc = 0;
//ca = cb = cc = 0;  // Error: expression cc.opAssign(0) is void and has no value
  ca = 0;  // have to do separately
  cb = 0;
  cc = 0;

  // 2. have to use .get to call math func
  writeln(std.math.sgn(la));
  writeln(std.math.abs(la));
//writeln(std.math.sgn(ca));  // Error: template std.math.sgn cannot deduce function from argument types !()(Checked!(long, Abort)), candidates are:
//writeln(std.math.abs(ca));  // Error: template instance std.math.abs!(Checked!(long, Abort)) error instantiating
  writeln(std.math.sgn(ca.get));  // need .get
  writeln(std.math.abs(ca.get));  // need .get

  // 3. no opBinaryLeft
  lc = la * lb;
//cc = la * cb;  // Error: can only * a pointer, not a int
  cc = cb * la;  // have to switch order

  // 4. std.conv don't work
  double d = 0;
  lb = d.to!long;
//cb = d.to!Long;  // Error: template instance std.conv.to!(Checked!(long, Abort)).to!double error instantiating
  cb = lround(d);

  lb = "0".to!long;
//cb = "0".to!Long; // Error: template std.conv.toImpl cannot deduce function from argument types !(Checked!(long, Abort))(string), candidates are:
  cb = "0".to!long; // work around ok

  // 5. cannot assign to shared
  static struct S {
    shared long sl;
    shared Long sL;
  }
  S s;
  s.sl = la;
//s.sL = la;  // Error: template std.experimental.checkedint.Checked!(long, Abort).Checked.opAssign cannot deduce function from argument types !()(long) shared, candidates are:
  cast()s.sL = la;  // need the `cast`

  // 6. format %d won't work
  writefln("%04d", la);      // output: 0000
//writefln("%04d", ca);      // Expected '%s' format specifier for type 'Checked!(long, Abort)'
  writefln("%s",   ca);      // output: Checked!(long, Abort)(0), can this be just be 0?
  writefln("%04d", ca.get);  // output: 0000

  // 7. atomic won't work
  core.atomic.atomicOp!"+="(s.sl, lb);
//core.atomic.atomicOp!"+="(s.sL, cb);  // Error: template instance core.atomic.atomicOp!("+=", Checked!(long, Abort), Checked!(long, Abort)) error instantiating
//core.atomic.atomicFetchAdd(s.sL, cb);  // Error: template core.atomic.atomicFetchAdd cannot deduce function from argument types !()(shared(Checked!(long, Abort)), Checked!(long, Abort)), candidates are:
}
------------------------------------------------------------------------

On Sat, Aug 15, 2020 at 04:28:05AM +0000, mw via Digitalmars-d wrote: [...]
>   // 1. checkedint cannot be chain assigned
>   la = lb = lc = 0;
> //ca = cb = cc = 0;  // Error: expression cc.opAssign(0) is void and has no
> value
>   ca = 0;  // have to do separately
>   cb = 0;
>   cc = 0;

This one should be easy to fix. Just change opAssign to return `this` instead of void, and that should solve the problem.


[...]
>   // 3. no opBinaryLeft
>   lc = la * lb;
> //cc = la * cb;  // Error: can only * a pointer, not a int
>   cc = cb * la;  // have to switch order

You mean lacking opBinaryRight? That should be simple to implement?


>   // 4. std.conv don't work
>   double d = 0;
>   lb = d.to!long;
> //cb = d.to!Long;  // Error: template instance std.conv.to!(Checked!(long,
> Abort)).to!double error instantiating
>   cb = lround(d);

Checked should implement `opCast(T : double)()`.


>   lb = "0".to!long;
> //cb = "0".to!Long; // Error: template std.conv.toImpl cannot deduce
> function from argument types !(Checked!(long, Abort))(string), candidates
> are:
>   cb = "0".to!long; // work around ok

Add a ctor to Checked that takes a string argument and parses it into a Checked value.


[...]
>   // 6. format %d won't work
>   writefln("%04d", la);      // output: 0000
> //writefln("%04d", ca);      // Expected '%s' format specifier for type
> 'Checked!(long, Abort)'
>   writefln("%s",   ca);      // output: Checked!(long, Abort)(0), can this
> be just be 0?
>   writefln("%04d", ca.get);  // output: 0000

Add an overload of toString that takes a FormatSpec argument:

	void toString(W,Char)(W sink, FormatSpec!Char fmt)
		if (isOutputRange!(W, Char))
	{
		if (fmt.spec == "d") { ... /* do the right thing here */ }
		... // etc.
	}

These should probably be done as a series of Phobos PRs to improve the usability of Checked.


T

-- 
What's a "hot crossed bun"? An angry rabbit.

More thoughts:

On Sat, Aug 15, 2020 at 04:28:05AM +0000, mw via Digitalmars-d wrote: [...]
>   // 2. have to use .get to call math func
>   writeln(std.math.sgn(la));
>   writeln(std.math.abs(la));
> //writeln(std.math.sgn(ca));  // Error: template std.math.sgn cannot deduce
> function from argument types !()(Checked!(long, Abort)), candidates are:
> //writeln(std.math.abs(ca));  // Error: template instance
> std.math.abs!(Checked!(long, Abort)) error instantiating
>   writeln(std.math.sgn(ca.get));  // need .get
>   writeln(std.math.abs(ca.get));  // need .get

Technically you *could* use `alias this` to decay it to long automatically, then these calls will work. But (1) that runs the risk of implicit conversion accidentally losing the guarantees of Checked when it silently turns into an unwrapped long, thereby defeating the purpose of using Checked; and (2) `alias this` is frowned upon these days because of its complicated relationship with subtyping.

One workaround is to write forwarding overloads for these functions as member functions of Checked.  But this is D, and we dislike boilerplate, so opDispatch comes to the rescue:

	struct Checked {
		...
		auto opDispatch(string memb)(Checked arg)
			if (memb == "sgn" || memb == "abs" || ... /* etc */)
		{
			return mixin(memb ~ "(this.get)");
		}
		...
	}

The bad thing is that you have to manually list the functions. But if then, you could import std.math and use introspection to extract the list of functions automatically.  Good opportunity to take advantage of D's metaprogramming capabilities.  Start with `__traits(allMembers, std.math)` and go from there.


T

-- 
Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? -- Michael Beibl

On 8/14/2020 9:28 PM, mw wrote:
> I list 8 things I've found below. Suggestions or PR welcome, e.g. even on how to improve the current implementation of `struct Checked`, or help from the compiler.

This is nice work. Thank you! Please file these issues in Bugzilla.

On Sunday, 16 August 2020 at 23:36:51 UTC, Walter Bright wrote:
> On 8/14/2020 9:28 PM, mw wrote:
>> I list 8 things I've found below. Suggestions or PR welcome, e.g. even on how to improve the current implementation of `struct Checked`, or help from the compiler.
>
> This is nice work. Thank you! Please file these issues in Bugzilla.

filed here:

https://issues.dlang.org/show_bug.cgi?id=21169

On Mon, Aug 17, 2020 at 05:10:01PM +0000, mw via Digitalmars-d wrote:
> On Sunday, 16 August 2020 at 23:36:51 UTC, Walter Bright wrote:
> > On 8/14/2020 9:28 PM, mw wrote:
> > > I list 8 things I've found below. Suggestions or PR welcome, e.g. even on how to improve the current implementation of `struct Checked`, or help from the compiler.
> > 
> > This is nice work. Thank you! Please file these issues in Bugzilla.
> 
> filed here:
> 
> https://issues.dlang.org/show_bug.cgi?id=21169

You might want to consider splitting up that bug into multiple smaller, more self-contained bugs.  Less intimidating that way for potential contributors to fix. ;-)


T

-- 
This sentence is false.

On Mon, Aug 17, 2020 at 05:10:01PM +0000, mw via Digitalmars-d wrote: [...]
> https://issues.dlang.org/show_bug.cgi?id=21169

Chain assignment fix: https://github.com/dlang/phobos/pull/7599


T

-- 
Some days you win; most days you lose.

On Monday, 17 August 2020 at 17:15:59 UTC, H. S. Teoh wrote:
> On Mon, Aug 17, 2020 at 05:10:01PM +0000, mw via Digitalmars-d wrote:
>> 
>> https://issues.dlang.org/show_bug.cgi?id=21169
>
> You might want to consider splitting up that bug into multiple smaller, more self-contained bugs.  Less intimidating that way for potential contributors to fix. ;-)

Was hesitating between splitting or not, then thought user may want to take the full picture into consideration the best approach to fix the issue, e.g. in case one-stone-that-can-kill-multiple-birds.

(People can always submit separate a PR to fix one particular issue).


> Chain assignment fix: https://github.com/dlang/phobos/pull/7599

Thanks for the PR, I just added comments: does this fix also work for mixed native & checked chain assignment? i.e. add to unittest:

```
  long la, lb;
  Checked!long ca, cb;

  la = ca = lb = cb;  // mixed chain assign
  ca = la = cb = lb;
```

On Mon, Aug 17, 2020 at 05:58:16PM +0000, mw via Digitalmars-d wrote:
> On Monday, 17 August 2020 at 17:15:59 UTC, H. S. Teoh wrote:
[...]
> > Chain assignment fix: https://github.com/dlang/phobos/pull/7599
> 
> Thanks for the PR, I just added comments: does this fix also work for mixed native & checked chain assignment? i.e. add to unittest:
> 
> ```
>   long la, lb;
>   Checked!long ca, cb;
> 
>   la = ca = lb = cb;  // mixed chain assign
>   ca = la = cb = lb;
> ```

Currently, it doesn't work.  I'm on the fence about whether it should: the whole point of using Checked is that you don't want to automatically convert to the native type because the converted value will lose the protections conferred by Check.  Assigning a Checked to a native type *might* be a mistake - you thought the variable was Checked but it wasn't, so subsequent operations on it no longer has Checked semantics even though you thought it does.  So I'm not sure this case should be supported.  Assigning from Checked back to native should always be explicit IMO (the programmer explicitly indicates he doesn't want Checked protections anymore).


T

-- 
If I were two-faced, would I be wearing this one? -- Abraham Lincoln

On Monday, 17 August 2020 at 18:10:16 UTC, H. S. Teoh wrote:
> On Mon, Aug 17, 2020 at 05:58:16PM +0000, mw via Digitalmars-d wrote:
>> On Monday, 17 August 2020 at 17:15:59 UTC, H. S. Teoh wrote:
> [...]
>> > Chain assignment fix: https://github.com/dlang/phobos/pull/7599
>> 
>> Thanks for the PR, I just added comments: does this fix also work for mixed native & checked chain assignment? i.e. add to unittest:
>> 
>> ```
>>   long la, lb;
>>   Checked!long ca, cb;
>> 
>>   la = ca = lb = cb;  // mixed chain assign
>>   ca = la = cb = lb;
>> ```
>
> Currently, it doesn't work.  I'm on the fence about whether it should: the whole point of using Checked is that you don't want to automatically convert to the native type because the converted value will lose the protections conferred by Check.  Assigning a Checked to a native type *might* be a mistake - you thought the variable was Checked but it wasn't, so subsequent operations on it no longer has Checked semantics even though

Yes, that's the principle we all agree. However, we are talking about opAssign() here.

The user specifies his/her intention via the variable's type declaration, e.g. native `long` vs checked `Long`. The *subsequent* operations you talking about will be on user specified variable (type), there will be no surprise here: if the LHS is declared as a `long`, the subsequent operations will be on `long`, and if the LHS is `Long`, the subsequent operations will be on `Long`, all as user has specified.

opAssign() just make the boxing/unboxing life easier between these two types. And there is not any mathematical operation performed inside opAssign(), hence for this particular function, native == checked is always true. So I think let opAssign() return the underlying type will make the drop-in replacement process more smooth, and without extra correctness concern.


> you thought it does.  So I'm not sure this case should be supported.  Assigning from Checked back to native should always be explicit IMO (the programmer explicitly indicates he doesn't want Checked protections anymore).