Re: unsigned char confusion

[Mike Gran]

On Tue, 2009-08-11 at 09:39 -0400, Greg Troxel wrote:
> In srfi-13.c line 25222, SCM_MAKE_CHAR is called with an argument that
> is an unsigned char.  This leads to:
> 
> cc1: warnings being treated as errors
> srfi-13.c: In function 'string_titlecase_x':
> srfi-13.c:2522: warning: comparison is always false due to limited range of 
> data type
> srfi-13.c:2522: warning: comparison is always false due to limited range of 
> data type
> 
> This is because SCM_MAKE_CHAR (in libguile/chars.h) has a bizarre
> conditional that checks the argument for < 0, and if so casts it to
> unsigned char.  Otherwise it does not cast.  There is no comment that
> explains what the point is.  Fairly obviously this is an attempt to
> avoid sign extension during SCM_MAKE_ITAG8.  The value is then cast to
> uintptr_t which is also unsigned, but sign extension would set more
> bits.
> 
> So, I think the cast to unsigned char should just always be there, without 
> the test.
> 

Yeah, that was me.   In the move to Unicode, I'm trying to get to a
point where the underlying storage of characters is uint32.  I was
trying to come up with a macro that would cast all of char, unsigned
char, and uint32 to uint32, since SCM_MAKE_CHAR is used in each of those
cases in the code.  If SCM_MAKE_CHAR receives something negative, it is
from a signed char.

For portability, it might be best if SCM_MAKE_CHAR becomes an inline
function that takes int32, since the top bit of uint32 isn't used in
encoding Unicode codepoints anyway.  That would cover all those cases.

Or, to save the macro, it could become

#define SCM_MAKE_CHAR(x)                                              \
  (((scm_t_int32) (x) < 0)                                            \
   ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (x), scm_tc8_char)  \
   : SCM_MAKE_ITAG8 ((scm_t_bits) (x), scm_tc8_char))

Yeah, and better comments for that as well.

-Mike