Do not rely on the result of a comma expression

[mathias-lang-sociomantic]

Will be required by dlang/dmd#5737

@Hackerpilot : Shouldn't dscanner have picked that up ?
Ping @DmitryOlshansky since inlining this part of the code seems critical.

[Hackerpilot]

Here's a quote from the config file in Phobos:

; Checks for use of the comma operator
comma_expression_check="disabled"

[schveiguy]

I'm... not liking the way this was refactored.

If I were to refactor, I'd do:

if(_index == _origin.length)
    return false;
else
{
    res = std.utf.decode(_origin, _index);
    return true;
}

I mean, this comma operator (ab)use is basically to save some braces. Just put them back.

[mathias-lang-sociomantic]

@schveiguy : That's what I originally went with, but it's not a single statement anymore.
Since the comment explicitly mention multiple returns as being awful for the inliner, I removed those.

[schveiguy]

Ugh, I hate fixing compiler/optimizer bugs with refactoring. Though I've done it in the past...

A comma-less similarly hacky fix:

return (res = std.utf.decode(_origin, _index)) || true;

[mathias-lang-sociomantic]

Ugh, I hate fixing compiler/optimizer bugs with refactoring.

Shared feeling. Though here it's just shifting a compiler workaround, not introducing a new one.

return (res = std.utf.decode(_origin, _index)) || true;

Considered that too, what if the string has embedded \0 ?

[mathias-lang-sociomantic]

Considered that too, what if the string has embedded \0 ?

I should double-read my messages...
This solution will works, although is it really better than the proposed version ?

[schveiguy]

although is it really better than the proposed version

The proposed version is hard to follow. It's crying for someone to refactor thinking "WTF is this?"
TBH, the only way to test what is "better" is to compile and see if it results in the same assembly. I think my version is easier to understand as a reviewer. If it results in the same as the original, I think we should go with that.

[mathias-lang-sociomantic]

Naive objdump --disassemble --demangle=dlang generated/linux/release/64/libphobos2.a:

master:

0000000000000000 <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)>:
   0:   55                      push   %rbp
   1:   48 8b ec                mov    %rsp,%rbp
   4:   48 83 ec 10             sub    $0x10,%rsp
   8:   48 89 5d f0             mov    %rbx,-0x10(%rbp)
   c:   48 89 55 f8             mov    %rdx,-0x8(%rbp)
  10:   48 8b cf                mov    %rdi,%rcx
  13:   48 8b 41 10             mov    0x10(%rcx),%rax
  17:   48 89 06                mov    %rax,(%rsi)
  1a:   48 8b 59 10             mov    0x10(%rcx),%rbx
  1e:   48 3b 19                cmp    (%rcx),%rbx
  21:   75 0b                   jne    2e <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)+0x2e>
  23:   31 c0                   xor    %eax,%eax
  25:   48 8b 5d f0             mov    -0x10(%rbp),%rbx
  29:   48 8b e5                mov    %rbp,%rsp
  2c:   5d                      pop    %rbp
  2d:   c3                      retq   
  2e:   48 89 ce                mov    %rcx,%rsi
  31:   48 8d 79 10             lea    0x10(%rcx),%rdi
  35:   e8 00 00 00 00          callq  3a <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)+0x3a>
  3a:   48 8b 4d f8             mov    -0x8(%rbp),%rcx
  3e:   89 01                   mov    %eax,(%rcx)
  40:   b8 01 00 00 00          mov    $0x1,%eax
  45:   48 8b 5d f0             mov    -0x10(%rbp),%rbx
  49:   48 8b e5                mov    %rbp,%rsp
  4c:   5d                      pop    %rbp
  4d:   c3                      retq   
  4e:   66 90                   xchg   %ax,%ax

-            return res = std.utf.decode(_origin, _index), true;
+            return res = std.utf.decode(_origin, _index) || true;

0000000000000000 <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)>:
   0:   55                      push   %rbp
   1:   48 8b ec                mov    %rsp,%rbp
   4:   48 83 ec 10             sub    $0x10,%rsp
   8:   48 89 5d f0             mov    %rbx,-0x10(%rbp)
   c:   48 89 55 f8             mov    %rdx,-0x8(%rbp)
  10:   48 8b cf                mov    %rdi,%rcx
  13:   48 8b 41 10             mov    0x10(%rcx),%rax
  17:   48 89 06                mov    %rax,(%rsi)
  1a:   48 8b 59 10             mov    0x10(%rcx),%rbx
  1e:   48 3b 19                cmp    (%rcx),%rbx
  21:   75 0b                   jne    2e <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)+0x2e>
  23:   31 c0                   xor    %eax,%eax
  25:   48 8b 5d f0             mov    -0x10(%rbp),%rbx
  29:   48 8b e5                mov    %rbp,%rsp
  2c:   5d                      pop    %rbp
  2d:   c3                      retq   
  2e:   48 89 ce                mov    %rcx,%rsi
  31:   48 8d 79 10             lea    0x10(%rcx),%rdi
  35:   e8 00 00 00 00          callq  3a <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)+0x3a>
  3a:   b9 01 00 00 00          mov    $0x1,%ecx
  3f:   81 e1 ff 00 00 00       and    $0xff,%ecx
  45:   48 8b 45 f8             mov    -0x8(%rbp),%rax
  49:   89 08                   mov    %ecx,(%rax)
  4b:   85 c9                   test   %ecx,%ecx
  4d:   0f 95 c0                setne  %al
  50:   48 8b 5d f0             mov    -0x10(%rbp),%rbx
  54:   48 8b e5                mov    %rbp,%rsp
  57:   5d                      pop    %rbp
  58:   c3                      retq   
  59:   0f 1f 80 00 00 00 00    nopl   0x0(%rax)

Proposed version:

0000000000000000 <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)>:
   0:   55                      push   %rbp
   1:   48 8b ec                mov    %rsp,%rbp
   4:   48 83 ec 20             sub    $0x20,%rsp
   8:   48 89 5d e8             mov    %rbx,-0x18(%rbp)
   c:   48 89 55 f8             mov    %rdx,-0x8(%rbp)
  10:   48 89 fb                mov    %rdi,%rbx
  13:   48 8b 43 10             mov    0x10(%rbx),%rax
  17:   48 89 06                mov    %rax,(%rsi)
  1a:   48 8b 4b 10             mov    0x10(%rbx),%rcx
  1e:   48 3b 0b                cmp    (%rbx),%rcx
  21:   41 0f 95 c0             setne  %r8b
  25:   44 88 45 f0             mov    %r8b,-0x10(%rbp)
  29:   74 12                   je     3d <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)+0x3d>
  2b:   48 89 fe                mov    %rdi,%rsi
  2e:   48 8d 7b 10             lea    0x10(%rbx),%rdi
  32:   e8 00 00 00 00          callq  37 <std.regex.internal.ir.Input!(char).Input.nextChar(ref dchar, ref ulong)+0x37>
  37:   48 8b 55 f8             mov    -0x8(%rbp),%rdx
  3b:   89 02                   mov    %eax,(%rdx)
  3d:   0f be 45 f0             movsbl -0x10(%rbp),%eax
  41:   48 8b 5d e8             mov    -0x18(%rbp),%rbx
  45:   48 8b e5                mov    %rbp,%rsp
  48:   5d                      pop    %rbp
  49:   c3                      retq   
  4a:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)

[schveiguy]

Meh, my version adds a couple of instructions (is it really moving 1 into ecx, and then anding 0xff with it?). Yours seems shorter than both, but I'm not an expert in what this means for actual speed. I think it's @DmitryOlshansky's call.

[DmitryOlshansky]

It doesn't matter how long nextChar is by itself - if it doesn't inline. And as far as I can tell it won't costing us 2-3% of speed of std.regex.

[DmitryOlshansky]

The re-write is fine, just please make sure it's inlined by DMD.

[wilzbach]

just please make sure it's inlined by DMD

Can't we do a simple benchmark of both versions to be sure?

[yebblies]

It should fail to compile with pragma(inline, true); if it doesn't work any more.

[DmitryOlshansky]

it should fail to compile with pragma(inline, true); if it doesn't work any more.

@mathias-lang-sociomantic Yes, please add pragma(inline, true) then.

[mathias-lang-sociomantic]

Done

[DmitryOlshansky]

Auto-merge toggled on