http://d.puremagic.com/issues/show_bug.cgi?id=2809

           Summary: Wrong code for unsigned shift
           Product: D
           Version: 2.027
          Platform: PC
               URL: http://www.digitalmars.com/webnews/newsgroups.php?art_gr
                    oup=digitalmars.D.learn&article_id=16107
        OS/Version: Windows
            Status: NEW
          Keywords: wrong-code
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: bugzilla@digitalmars.com
        ReportedBy: maxmo@pochta.ru


---
const short s=-1;
static assert(s>>>1==0x7fff); //fail
---

Influenced by error messages, where compiler transforms a>>>b to
cast(int)a>>>b.
Here compiler complains about conversion to return type:
---
short a(short b) { return b>>>1; }
---
When you add it, the code is accepted, but the bug is already triggered.
---
short a(short b) { return cast(short)(b>>>1); }
---


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=2809


Don <clugdbug@yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |clugdbug@yahoo.com.au
            Version|2.027                       |1.00
            Summary|Wrong code for unsigned     |Wrong constant folding for
                   |shift                       |unsigned shift


--- Comment #1 from Don <clugdbug@yahoo.com.au> 2010-01-16 23:37:20 PST ---
Also applies to D1. Seems to be a constant folding issue.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=2809


Don <clugdbug@yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch


--- Comment #2 from Don <clugdbug@yahoo.com.au> 2010-01-17 11:48:08 PST ---
Now I'm really confused. In Walter's test suite, this case is explicitly tested!

static assert((cast(short)-1 >>> 1) == int.max);

There's a wrong statement in the bug description.
"Here compiler complains about conversion to return type:
---
short a(short b) { return b>>>1; } "

the response to this should be:
short a(short b) { return b>>>cast(short)1; }

So I'm rather confused about whether this is a bug, or intended (but
unintuitive) behaviour.

If it's a bug, it can be fixed by modifying UshrExp::semantic(Scope *sc) in
expression.c (around line 10103):
    e1 = e1->checkIntegral();
    e2 = e2->checkIntegral();
-    e1 = e1->integralPromotions(sc);
+    e = e1->integralPromotions(sc);
    e2 = e2->castTo(sc, Type::tshiftcnt);
-    type = e1->type;
+    type = e->type;
    }
    return this;

and in constfold.c Ushr(), around line 600, removing two asserts.
    case Tint8:
    case Tuns8:
-        assert(0);        // no way to trigger this
        value = (value & 0xFF) >> count;
        break;

    case Tint16:
    case Tuns16:
-        assert(0);        // no way to trigger this
        value = (value & 0xFFFF) >> count;
        break;

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=2809


simon <s.d.hammett@googlemail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |s.d.hammett@googlemail.com


--- Comment #3 from simon <s.d.hammett@googlemail.com> 2010-11-15 14:29:19 PST ---
Mr Bs test case is wrong:

static assert((cast(short)-1 >>> 1) == int.max);

should be:

static assert((cast(short)-1 >>> 1) == short.max);

unsigned right shift is perfectly well defined,
though giving it it's own operator seems like overkill.

I think it would be better as a function in std.intrinsic.

You aren't going to use unsigned shift unless you know what you doing and care about performance.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=2809



--- Comment #4 from Don <clugdbug@yahoo.com.au> 2010-11-15 15:06:34 PST ---
(In reply to comment #3)
> Mr Bs test case is wrong:
> 
> static assert((cast(short)-1 >>> 1) == int.max);
> 
> should be:
> 
> static assert((cast(short)-1 >>> 1) == short.max);

Not so. You might be thinking of this, which _is_ true:
static assert((cast(short)-1 >>> cast(short)1) == short.max);

The problem is that >>> interacts badly with implicit type conversions. With every other operator,  typeof(short OP int) == int.

Possible solutions are:
(a) special case for >>>
(b) disallow >>> for types smaller than int
(c) drop it from the language
Personally I think (c) is the only option that makes sense.

> unsigned right shift is perfectly well defined,
> though giving it it's own operator seems like overkill.
> 
> I think it would be better as a function in std.intrinsic.

You don't need it at all. Just cast to unsigned, then >>.
>>> is a ridiculous operator.

> You aren't going to use unsigned shift unless you know what you doing and care about performance.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=2809



--- Comment #5 from Sobirari Muhomori <dfj1esp02@sneakemail.com> 2010-11-16 11:53:02 PST ---
>short a(short b) { return b>>>cast(short)1; }
Shouldn't number literals work as smallest possible type and promoted as
needed?
Like here:
---
byte a=1;
---

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=2809



--- Comment #6 from Sobirari Muhomori <dfj1esp02@sneakemail.com> 2010-11-16 11:59:34 PST ---
Number literals are polysemous, right? So binary ops should work like this:
opBinary(l,r)
{
  if(is(typeof(r)==polysemous))
  {
    opBinary(l,implicit_cast(typeof(l))r);
  }
}

or something like that.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=2809



--- Comment #7 from Sobirari Muhomori <dfj1esp02@sneakemail.com> 2010-11-16 12:34:12 PST ---
> You don't need it at all. Just cast to unsigned, then >>.
> >>> is a ridiculous operator.

See bug 5225.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------