bug#32463: 27.0.50; (logior -1) => 4611686018427387903

[Andy Moreton <andrewjmoreton@gmail.com>]

On Fri 17 Aug 2018, Pip Cet wrote:

> Can you try the attached patch? It fixes a few things that I reported
> to Tom yesterday:
>
> ---
>
>
> None of these are necessarily bugs, but:
>
> * I find the behavior of `lsh', `logand', `logior', and `logxor', when
> given negative arguments, surprising. I think it would make most sense
> to treat negative numbers as the infinite bitstream consisting of all
> ones to the left of the specified value:
>
> (logand -1 -1) would be interpreted as ...1111111 & ...1111111 =
> ...1111111, so it would be -1 (rather than 2 * most-positive-fixnum +
> 1).
> (lsh (- (lsh -1 64) 1) -1) would be ...1110111...1111 shifted to the
> right by one digit, an odd number, rather than the even number
> currently produced. (I believe lsh and ash should behave identically.)

> @@ -3383,8 +3383,6 @@ ash_lsh_impl (Lisp_Object value, Lisp_Object count, bool lsh)
>        mpz_init (result);
>        if (XFIXNUM (count) >= 0)
>  	mpz_mul_2exp (result, XBIGNUM (value)->value, XFIXNUM (count));
> -      else if (lsh)
> -	mpz_tdiv_q_2exp (result, XBIGNUM (value)->value, - XFIXNUM (count));
>        else
>  	mpz_fdiv_q_2exp (result, XBIGNUM (value)->value, - XFIXNUM (count));
>        val = make_number (result);
> @@ -3401,14 +3399,7 @@ ash_lsh_impl (Lisp_Object value, Lisp_Object count, bool lsh)

Please add more test cases to test/src/data-tests.el to ensure that the
logical operations have the expected behaviour for positive and negative
arguments, and for both bignum and fixnum arguments. The tdiv/fdiv were
added to give expected results. Pay particular attention to values around
most-positive-fixnum and most-negative-fixnum.

>        if (XFIXNUM (count) >= 0)
>  	mpz_mul_2exp (result, result, XFIXNUM (count));
> -      else if (lsh)
> -	{
> -	  if (mpz_sgn (result) > 0)
> -	    mpz_fdiv_q_2exp (result, result, - XFIXNUM (count));
> -	  else
> -	    mpz_fdiv_q_2exp (result, result, - XFIXNUM (count));
> -	}
> -      else /* ash */
> +      else
>  	mpz_fdiv_q_2exp (result, result, - XFIXNUM (count));

This part is an obvious simplification and is fine (I should have
spotted it before sending my patch that Tom committed).

    AndyM