djimenez / iconv-go

iconv support for Go
BSD 2-Clause "Simplified" License
418 stars 66 forks source link

Fix Panic in Go 1.6 #24

Closed kowalczykp closed 8 years ago

kowalczykp commented 8 years ago

This should fix #23 unfortunately it uses additional allocation using malloc, but I didn't come up with a different solution to avoid passing pointer to go pointer into iconv (cgo). I tried to stick to the naming convention used in the code, although it became a bit too long around shift tracking.

Existent tests and examples seem to be working now both with go <1.6 and 1.6

kowalczykp commented 8 years ago

sorry, for late answer - been busy.

@dimandzhi Thanks for pointing that out :+1: Storing a pointer in C was a great hint, although cgo is not guaranteed to run in single thread even with GOMAXPROC==1, so having a global pointer in C and returning it's address would lead to panics. Alternative would be to make some table in C to store the pointers and make it thread-safe, but that would be still messy.

Apparently the simplest thing to be done, was to write a wrapper around iconv, which allows to omit the unnecessary allocations and go back to previous way of handling pointer shifting, without the mess I introduced ;)

size_t call_iconv(iconv_t ctx, char *in, size_t *size_in, char *out, size_t *sizeout){
    return iconv(ctx, &in, size_in, &out, sizeout);
}

As for your last question, passing a slice as a parameter into function does not involve any copying of the slice content.

A slice is a descriptor of an array segment. It consists of a pointer to the array, the length of the segment, and its capacity (the maximum length of the segment).

djimenez commented 8 years ago

what you did with call_iconv wrapper looks like what I expected would end up being the solution i thnk, so now I have confirmation bias.

can you squash your commits and reference the issue number in the commit message?

kowalczykp commented 8 years ago

Since, the approach (fortunately) changed, the byteSliceToC helper func could have been omitted as well as the white space

If that bothers you I can remove both with another squashing

djimenez commented 8 years ago

the extra changes did bug me a little, I will not complain if you amend them out.

Thanks.

kowalczykp commented 8 years ago

adjusted :)