Re: [PATCH] Simplify the implementation of ios_mask functions.

[Egeyar Bagcioglu]

Hi John,

Thanks for the patch that's removing my unnecessary switches :)
I have some worries about the correctness. Please take a look at the 
comment below.


On 12/18/19 5:18 PM, John Darrington wrote:
> ---
>   ChangeLog |  5 +++++
>   src/ios.c | 60 ++----------------------------------------------------------
>   2 files changed, 7 insertions(+), 58 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index f21d5d3..c7d4a78 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2019-12-15 John Darrington <address@hidden>
> +
> +       *src/ios.c (ios_mask_last_byte) simplify
> +       *src/ios.c (ios_mask_first_byte) simplify
> +
>   2019-12-16  Jose E. Marchesi  <address@hidden>
>   
>   	* src/pkl-asm.pks (atrim): The mka instruction expects the type of
> diff --git a/src/ios.c b/src/ios.c
> index a96e349..e95e767 100644
> --- a/src/ios.c
> +++ b/src/ios.c
> @@ -313,69 +313,13 @@ ios_map (ios_map_fn cb, void *data)
>   inline static void
>   ios_mask_first_byte(uint64_t *byte, int significant_bits)
>   {
> -  switch (significant_bits)
> -    {
> -    case 1:
> -      *byte &= (char) 0x01;
> -      return;
> -    case 2:
> -      *byte &= (char) 0x03;
> -      return;
> -    case 3:
> -      *byte &= (char) 0x07;
> -      return;
> -    case 4:
> -      *byte &= (char) 0x0f;
> -      return;
> -    case 5:
> -      *byte &= (char) 0x1f;
> -      return;
> -    case 6:
> -      *byte &= (char) 0x3f;
> -      return;
> -    case 7:
> -      *byte &= (char) 0x7f;
> -      return;
> -    case 8:
> -      return;
> -    default:
> -      assert (0);
> -      return;
> -    }
> +  *byte &= ~0 >> 8 - significant_bits;

The right-hand side of the statement above will create a number with "8 
- significant_bits" many leading 0s followed by many 1s. Since "byte" 
has only its least significant byte set, this will leave it unchanged. 
What we want here instead is to and with significant_bits many 1s in the 
least significant bits, with as many leading 0s as necessary. I am being 
picky as this is a matter of correctness of the code, in spite of poke 
VM also fixing such errors.

Can we use the following instead?

*byte &= (1 << significant_bits) - 1;

What do you think?

>   }
>   
>   inline static void
>   ios_mask_last_byte(uint64_t *byte, int significant_bits)
>   {
> -  switch (significant_bits)
> -    {
> -    case 1:
> -      *byte &= (char) 0x80;
> -      return;
> -    case 2:
> -      *byte &= (char) 0xc0;
> -      return;
> -    case 3:
> -      *byte &= (char) 0xe0;
> -      return;
> -    case 4:
> -      *byte &= (char) 0xf0;
> -      return;
> -    case 5:
> -      *byte &= (char) 0xf8;
> -      return;
> -    case 6:
> -      *byte &= (char) 0xfc;
> -      return;
> -    case 7:
> -      *byte &= (char) 0xfe;
> -      return;
> -    case 8:
> -      return;
> -    default:
> -      assert (0);
> -      return;
> -    }
> +  *byte &= ~0 << 8 - significant_bits;
>   }

This one is perfect. Thanks!

>   
>   static inline int

With the suggested change above, the changes are okay for me. Although I 
should thank Jose for caring for my opinion regarding changes that get 
into these functions, I should also remind you that I am neither a 
global reviewer nor a maintainer of this code, you'll still need Jose's 
review in the end.

I also considered your objection on such functions operation on uint64_t 
and how that is confusing. I think the best is to turn them into macros. 
If you don't beat me to it, I will do so the on the first days of 2020.

As you pointed out, test cases not showing such failures is also 
problematic. After finally implementing ios_write functions, my 
intention is to move on to adding some unit tests for the ios functions 
exactly because of this reason. I will need some directions and 
suggestions to do so. I believe the timing of the GHM in Mont Soleil 
will be pretty good for that. I hope you can help me with that too.

Regards
Egeyar