On 07/02/2016 02:45 AM, Patrick Schluter wrote:
> On Friday, 1 July 2016 at 20:09:30 UTC, Andrei Alexandrescu wrote:
>> On 7/1/16 3:43 PM, Patrick Schluter wrote:
>>> On Friday, 1 July 2016 at 16:30:41 UTC, Andrei Alexandrescu wrote:
>>>> https://issues.dlang.org/show_bug.cgi?id=16224 -- Andrei
>>>
>>> That
>>>
>>>     do {
>>>     } while(0)
>>>
>>> construct is ridiculous. It's cargo cult at its worst.
>>
>> That escalated quickly. -- Andrei
>
> Nothing personal. As I said in the initial message, I had to put up with
> that construct for the last 15 years because my colleague loves it. So I
> had ample time to learn to hate it with a passion.

How would you reshape this? It's important that the call to hook is physically at the end of the function and issued just in that place, and that the code does not do any redundant work.

U hook(U, T)(T value) { return U.init; }

U opCast(U, T)(T payload)
{
	import std.traits;
	do
	{
		static if (!isUnsigned!T && isUnsigned!U &&
				   T.sizeof <= U.sizeof)
		{
			if (payload < 0) break; // extra check needed
		}
		auto result = cast(U) payload;
		if (result != payload) break;
		static if (isUnsigned!T && !isUnsigned!U)
		{
			static assert(U.sizeof <= T.sizeof);
			if (result < 0) break;
		}
		return result;
	} while (0);
	return hook!U(payload);
}

void main()
{
	opCast!short(42);
}


Thanks,

Andrei

On Saturday, 2 July 2016 at 12:26:34 UTC, Andrei Alexandrescu wrote:
>
> How would you reshape this?

Maybe:

U opCast(U, T)(T payload)
{
    import std.traits;

    enum descriptiveName = !isUnsigned!T && isUnsigned!U && T.sizeof <= U.sizeof;
    enum unsT_sigU = isUnsigned!T && !isUnsigned!U;
    static if (unsT_sigU)
    {
        static assert(U.sizeof <= T.sizeof);
    }

    if (!descriptiveName || payload >= 0)
    {
        auto result = cast(U) payload;
        if (result == payload)
        {
            if (!unsT_sigU || result >= 0)
            {
                return result;
            }
        }
    }

    return hook!U(payload);
}

On 07/02/2016 09:23 AM, Johan Engelen wrote:
> On Saturday, 2 July 2016 at 12:26:34 UTC, Andrei Alexandrescu wrote:
>>
>> How would you reshape this?
>
> Maybe:
>
> U opCast(U, T)(T payload)
> {
>      import std.traits;
>
>      enum descriptiveName = !isUnsigned!T && isUnsigned!U && T.sizeof <=
> U.sizeof;
>      enum unsT_sigU = isUnsigned!T && !isUnsigned!U;
>      static if (unsT_sigU)
>      {
>          static assert(U.sizeof <= T.sizeof);
>      }
>
>      if (!descriptiveName || payload >= 0)
>      {
>          auto result = cast(U) payload;
>          if (result == payload)
>          {
>              if (!unsT_sigU || result >= 0)
>              {
>                  return result;
>              }
>          }
>      }
>
>      return hook!U(payload);
> }

Nice, thanks. I've tried to not rely too much on mixing statically-known and dynamically-known Boolean expressions. Can I safely assume that all compilers will generate good code for such? According to asm.dlang.org, dmd generates identical code at least for the two functions above. -- Andrei

On Saturday, 2 July 2016 at 12:26:34 UTC, Andrei Alexandrescu wrote:
> How would you reshape this? It's important that the call to hook is physically at the end of the function and issued just in that place, and that the code does not do any redundant work.

Your function does redundant work.  E.g. opCast!(int, ubyte) should not require any checks.  I also don't understand why opCast!(int, ubyte) is not allowed.

The following code should do the same as yours, but without unnecessary checks:

U opCast(U, T)(T payload)
{
        import std.traits;
        enum Tsizeof = is(T==bool) ? 0 : T.sizeof;
        enum Usizeof = is(U==bool) ? 0 : U.sizeof;
        enum noCheck = isUnsigned!T == isUnsigned!U && Tsizeof <= Usizeof ||
                       isUnsigned!T && Tsizeof < Usizeof;
        enum checkPayload = !isUnsigned!T && isUnsigned!U;
        enum checkResult = isUnsigned!T && !isUnsigned!U;
        static if (checkResult)
        {
                static assert(U.sizeof <= T.sizeof);  // I don't understand this
        }

        if (!checkPayload || payload >= 0)
        {
                auto result = cast(U) payload;
                if (noCheck || result == payload && (!checkResult || result >= 0))
                        return result;
        }
        return hook!U(payload);
}

On Saturday, 2 July 2016 at 15:15:39 UTC, Guillaume Boucher wrote:
> E.g. opCast!(int, ubyte) should not require any checks.

Or opCast!(long, int) and opCast!(int, int).

On 07/02/2016 11:15 AM, Guillaume Boucher wrote:
> Your function does redundant work.  E.g. opCast!(int, ubyte) should not
> require any checks.  I also don't understand why opCast!(int, ubyte) is
> not allowed.

It's an adapted fragment from a larger function that handles other cases separately. -- Andrei

On Saturday, 2 July 2016 at 14:03:29 UTC, Andrei Alexandrescu wrote:
>
> Nice, thanks. I've tried to not rely too much on mixing statically-known and dynamically-known Boolean expressions. Can I safely assume that all compilers will generate good code for such?

I'd be very surprised if not (especially with statically-known booleans upfront in the expression).

I get this for opCast!(short, int)(int):

asm.dlang.org ("-O -inline")
    push   %rax
    mov    %rdi,%rcx
    movswl %cx,%eax
    cmp    %ecx,%eax
    jne    label1
    mov    %rdi,%rax
    jmp    label2
    label1:
    xor    %eax,%eax
    label2:
    pop    %rcx
    retq

LDC trunk (-O3)  (identical code for your and my version)
    movswl  %di, %eax
    cmpl      %edi, %eax
    jne         LBB2_1
    movswl  %di, %eax
    retq
    LBB2_1:
    xorl        %eax, %eax
    retq

Define "good" ;-) ;-)

PS: you can look at LDC's annotated asm output with "-c -output-s".

On Saturday, 2 July 2016 at 15:15:39 UTC, Guillaume Boucher wrote:
> U opCast(U, T)(T payload)
> {
>         import std.traits;
>         enum Tsizeof = is(T==bool) ? 0 : T.sizeof;
>         enum Usizeof = is(U==bool) ? 0 : U.sizeof;
>         enum noCheck = isUnsigned!T == isUnsigned!U && Tsizeof <= Usizeof ||
>                        isUnsigned!T && Tsizeof < Usizeof;
>         enum checkPayload = !isUnsigned!T && isUnsigned!U;
>         enum checkResult = isUnsigned!T && !isUnsigned!U;
>         static if (checkResult)
>         {
>                 static assert(U.sizeof <= T.sizeof);  // I don't understand this
>         }
>
>         if (!checkPayload || payload >= 0)
>         {
>                 auto result = cast(U) payload;
>                 if (noCheck || result == payload && (!checkResult || result >= 0))
>                         return result;
>         }
>         return hook!U(payload);
> }

I got to something similar (probably with some typos), assuming .sizeof exists:


U opCast(U, T)(T payload)
{
    import std.traits;

    enum unsigned_to_signed =  isUnsigned!T && !isUnsigned!U;
    enum signed_to_unsigned =  !isUnsigned!T && isUnsigned!U;
    enum maybe_to_smaller = T.sizeof >= U.sizeof;
    enum to_smaller = T.sizeof > U.sizeof;

    static assert( !unsigned_to_signed || maybe_to_smaller);

    if( !signed_to_unsigned || to_smaller || payload >= 0 )
    {
        auto result = cast(U) payload;
        if (result == payload && !( unsigned_to_signed && result < 0))
            return result;
    }

    return hook!U(payload);
}

On 02.07.2016 14:26, Andrei Alexandrescu wrote:
>
> How would you reshape this? It's important that the call to hook is
> physically at the end of the function and issued just in that place, and
> that the code does not do any redundant work.
>
> U hook(U, T)(T value) { return U.init; }
>
> U opCast(U, T)(T payload)
> {
>      import std.traits;
>      do
>      {
>          static if (!isUnsigned!T && isUnsigned!U &&
>                     T.sizeof <= U.sizeof)
>          {
>              if (payload < 0) break; // extra check needed
>          }
>          auto result = cast(U) payload;
>          if (result != payload) break;
>          static if (isUnsigned!T && !isUnsigned!U)
>          {
>              static assert(U.sizeof <= T.sizeof);
>              if (result < 0) break;
>          }
>          return result;
>      } while (0);
>      return hook!U(payload);
> }
>
> void main()
> {
>      opCast!short(42);
> }
>
>
> Thanks,
>
> Andrei

How do you decide what 'redundant work' is? Is this combination of branches and type casts really particularly cheap to execute? Could masking out the "sign bit" from the unsigned value before comparison be faster? Also, isn't the result != payload check redundant if sizeof(U) == sizeof(T)?

Anyway, this is the kind of optimization that compilers are supposed to be good at, it should pick whatever procedure is optimal, not necessarily the one specified. Personally, I'd just write something like:

U opCast(U,T)(T payload)if(...){
    auto result = cast(U)payload;
    if(result == payload && (result<0) == (payload<0))
        return result;
    return hook!U(payload);
}

On Sunday, 3 July 2016 at 00:57:04 UTC, Timon Gehr wrote:
> How do you decide what 'redundant work' is? Is this combination of branches and type casts really particularly cheap to execute?

He just provided an example, so preserving the  essence of the logic for all kinds of U and T is important. The sizeof test is flawed in general (it does not reflect the maximum for all types on all hardware).

The question was more whether "break" usually is more clear and performant for this type of code. And the answer is no...