Hi Corinna,

On 16.08.2012 11:33, Corinna Vinschen wrote:
> Hi Thomas,
>
> thanks for the patch.   I have a few minor nits:
>
> On Aug 14 22:56, Thomas Wolff wrote:
>> --- sav/fhandler_clipboard.cc	2012-07-08 02:36:47.000000000 +0200
>> +++ ./fhandler_clipboard.cc	2012-08-14 18:25:14.903255600 +0200
>> ...
> See (*) below.
>
>> ...
>> +	  char * _ptr = (char *) ptr;
>> +	  size_t _len = len;
> I would prefer to have local variable names here which don't just
> differ by a leading underscore.  It's a bit confusing.  What about,
> say, tmp_ptr/tmp_len, or use_ptr/use_len or something like that?
tmp_OK

>> +	  char cprabuf [8 + 1];	/* need this length for surrogates */
>> +	  if (len < 8)
>> +	    {
>> +	      _ptr = cprabuf;
>> +	      _len = 8;
>> +	    }
> 8?  Why 8?  The size appears to be rather artificial.  The code should
> use MB_CUR_MAX instead.
MB_CUR_MAX does not work because its value is 1 at this point
(after adding #include <stdlib.h> anyway).
I guess that's because it changes its value after setlocale().
To avoid interference with application invocation of setlocale(),
however, it must not be called here (with which parameters anyway...).

So the maximum length of all encodings needs to be used here which
would be 4 (UTF-8, EUC-TW, GB 18030) if there were not the surrogate
pairs of UTF-16.
Since a single surrogate can be encoded with 3 UTF-8 bytes, I tried
6 to host two of them but it didn't work. So I increased to 8
which seemed to work; I may have been mislead, however, since another
test case now shows that the patch does not always work with surrogates.
It only works if the non-BMP character (the surrogate pair) does not
extend over the border of two blocks of multiple size of these internal
small buffers..., meaning e.g. if the buffer size is 8 and a non-BMP
character starts at byte 7 of the clipboard, it will fail (only the
second surrogate will be pasted).
I don't know what to do about this right now; maybe some tweak
of sys_wcstombs would help, it should perhaps convert to non-surrogate
UTF-8 first before any other buffering considerations.

--- Actually, having written all this, I just notice that surrogate pairs
don't work with the "old" code either (using a larger caller buffer)
whenever the character is not block-aligned as described above.
So maybe for the current patch, 4 would indeed be good, while the
surrogate problem should be fixed in sys_wcstombs.
Update attached.

>> ...
>> +	      /* If using read-ahead buffer, copy to class read-ahead buffer
>> +	         and deliver first byte. */
>> +	      if (_ptr == cprabuf)
>> +		{
>> +		  puts_readahead (cprabuf, ret);
>> +		  * (char *) ptr = get_readahead ();
>> +		  ret = 1;
> (*) Ok, that works, but wouldn't it be more efficient to do that in
> a tiny loop along the lines of
>
> 		  int x;
> 		  ret = 0;
>                    while (ret < len && (x = get_readahead ()) >= 0)
> 		    ptr++ = x;
> 		    ret++;
>
> ?
I can add it if you prefer; I just didn't think it's worth the effort 
and concerning efficiency, after that prior trial-and-error 
count-down-loop...

------
Thomas