www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 2809] New: Wrong code for unsigned shift

reply d-bugmail puremagic.com writes:
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); }
---


--
Apr 06 2009
next sibling parent d-bugmail puremagic.com writes:
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



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: -------
Jan 16 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2809


Don <clugdbug yahoo.com.au> changed:

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



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: -------
Jan 17 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
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



---
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: -------
Nov 15 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2809






 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: -------
Nov 15 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2809




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: -------
Nov 16 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2809




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: -------
Nov 16 2010
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2809




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: -------
Nov 16 2010