GNOME Bugzilla – Bug 515408
casting int[] to uchar[] breaks .length
Last modified: 2012-08-01 10:15:57 UTC
Please describe the problem: When casting int[] to uchar[] the length property is set to -1, which leads to problems when vala wants to be smart when passing arrays + length to C-code libs. This test program is expected to work when this bug has been fixed: public class Test { public static void main(string[] args) { int[] tmp = new int[] {1,2,3,4,5}; weak uchar[] test = (uchar[]) tmp; GLib.assert(test.length == tmp.length * (sizeof(int)/sizeof(uchar))); } } Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Confirming.
I am not sure the current behaviour is too bad, it does get the length nowadays (0.7.9) test = (_tmp2_ = (guchar*) tmp, test_length1 = tmp_length1, test_size = test_length1, _tmp2_); However, the cast is very straight, there is not type conversion. I am not sure if the bug is about: - length being -1 after cast: this is solved - cast being not a type conversion: do we want that? - if we don't want that, do we want to recompute length the way you proposed? (I don't like the idea so much.. it's too low level) marking as need info, I would reopen a bug to do type conversion, if it's what we want.
Just re-ran the test case, adding the following: GLib.debug("test.length = %d, tmp.length = %d", test.length, tmp.length); This yields: ** (process:16533): DEBUG: foo.vala:5: test.length = 5, tmp.length = 5 ** ERROR:/tmp/foo.vala.c:71:test_main: assertion failed: (test_length1 == (tmp_length1 * (sizeof (gint) / sizeof (guchar)))) So it's less broken now (length is initialized), but it's still not set to its correct value.
(In reply to comment #3) > ERROR:/tmp/foo.vala.c:71:test_main: assertion failed: (test_length1 == > (tmp_length1 * (sizeof (gint) / sizeof (guchar)))) > > > So it's less broken now (length is initialized), but it's still not set to its > correct value. What is correct? I disagree with your expectation. I would expect type conversion, you would expect array length change.
Is type conversion actually of any use there? My vote goes for array length change, because IMO it's a typecast of the array as a whole, not a conversion of it's elements.
Marc-Andre Lureau: If I have (In reply to comment #4) > > So it's less broken now (length is initialized), but it's still not set to its > > correct value. > > What is correct? I disagree with your expectation. I would expect type > conversion, you would expect array length change. Say that I cast an array of uint8 to an array of uint64, and keep the length like you suggest. Then I will read, and perhaps even write, outside the buffer if I let Vala trust the length that comes along with the variable. Is this recommended behavior according to you? Correct is when the test passes, otherwise I wouldn't have attached the test in the first place. Unfortunatly I'm not skilled enough to fix it myself, but I stand by the opinion that until this test case passes, Vala is broken.
Here is one example affected by this bug: http://git.xmms.se/?p=abraca.git;a=blob;f=src/components/playlist/playlist_view.vala#l513 And here's another: http://git.xmms.se/?p=abraca.git;a=blob;f=src/components/filter/filter_view.vala#l500 Both cases of horrible code, and I would be glad to find some better way of doing it, so if you have any suggestions....
Actually, what Marc-Andre suggests (correct me if I'm wrong, but any other interpretation wouldn't make sense) is that the cast should allocate a new array, iterate though the old one and do one-by-one conversion of the content. something like this: int* converted_array = malloc (orig_array_length * sizeof(int)); for (int i = 0; i < orig_array_length; ++i) { converted_array[i] = (int) orig_array[i]; } I don't think we want to do this automatically.
(In reply to comment #8) > Actually, what Marc-Andre suggests (correct me if I'm wrong, but any other > interpretation wouldn't make sense) is that the cast should allocate a new > array, iterate though the old one and do one-by-one conversion of the content. yes > I don't think we want to do this automatically. maybe not Perhaps the cast of array of different inner type should be forbidden anyway.
How would (In reply to comment #9) > > I don't think we want to do this automatically. > > maybe not > > Perhaps the cast of array of different inner type should be forbidden anyway. How would I pass data in drag-n-drop situations without this then?
(In reply to comment #10) > > Perhaps the cast of array of different inner type should be forbidden anyway. > I meant different "inner type size", sorry. But that's not an idea I would like - it's too low-level. > How would I pass data in drag-n-drop situations without this then? Create an array of desired types, and loop over all the src array values, with a cast? However, I still think it's the best, to create a new array and cast each value separately, keeping original array size. That would be consistent with other casts (such as Value to simple type) and be high level.
(In reply to comment #11) Value is a special case. It is actually consistent now, since it doesn't work, just like any other conversion between arbitrary struct types. :)) No, I'm not joking. At the moment, you can't even cast two exactly same struct types. gcc will spit an error, which should probably be a separate bug report. Simply recomputing length would be consistent with what you'd expect from a cast between structs. I completely disagree with an argument "it is too low level". It is perfectly the behavior one would expect from it. Consistent with both structure casts and pointer casts. Not to say there wouldn't be any alternative if you wanted a low-level cast. My proposal: Fix the casts as low-level. Implement a generic method for arrays for element conversion. E.g. int[] arr1; uint8[] arr2 = arr1.convert<uint8> (); That would IMO make much more sense.
(In reply to comment #11) > However, I still think it's the best, to create a new array and cast each value > separately, keeping original array size. Ok, so in this case I have a list of int32, and GtkSelectionData has this signature: void gtk_selection_data_set (GtkSelectionData *selection_data, GdkAtom type, gint format, const guchar *data, gint length); I can't really loop over my list of int32's and cast them to uchar.
(In reply to comment #13) > (In reply to comment #11) > I can't really loop over my list of int32's and cast them to uchar. then I would cast the pointer if you want to bypass conversion, and frankly I wonder why it does not take a pointer. Documentation says: data pointer to the data (will be copied) length of the data
(In reply to comment #2) > marking as need info, I would reopen a bug to do type conversion, if it's what > we want. So, any decision on this? I'm a bit confused by the discussion, so Marc-Andre, could you clear up?
(In reply to comment #15) > (In reply to comment #2) > > marking as need info, I would reopen a bug to do type conversion, if it's what > > we want. > So, any decision on this? I'm a bit confused by the discussion, so Marc-Andre, > could you clear up? the last proposal from Jiří makes sense.
So I'm reopening then since I don't see any open question.
Created attachment 219156 [details] [review] Recompute length when casting between array types Partially fixes bug 515408
Thanks for the patch. I agree that this is the right fix for this bug. Adding a higher level conversion function should be tracked in a separate bug/enhancement report if desired. For future patches, please follow the Vala coding style. commit d5f417c12f02d56bc377aaacc5e7b299525586cb Author: Simon Werbeck <simon.werbeck@gmail.com> Date: Wed Jul 18 22:11:20 2012 +0200 Recompute length when casting between array types This patch makes casting, e.g., from int[] to uchar[] work as expected by adjusting the array length field. Fixes bug 515408.