GNOME Bugzilla – Bug 602640
Array lengths are bogus at times
Last modified: 2009-12-22 17:00:29 UTC
The array field 'length' starts to count from the C arg list, so need to decrement when it's a method
Created attachment 148276 [details] [review] The array field 'length' starts to count from the C arg list, so need to decrement when it's a method
Review of attachment 148276 [details] [review]: Looks almost OK, but there is much more in this patch than what is needed to fix the bug; please clean it and open other bugs for the rest. Thanks!
Created attachment 148285 [details] [review] The array field 'length' starts to count from the C arg list, so need to decrement when it's a method
(In reply to comment #2) > Review of attachment 148276 [details] [review]: > > Looks almost OK, but there is much more in this patch than what is needed to > fix the bug; please clean it and open other bugs for the rest. Thanks! True, that stuff seems to be about arrays containing structs.
Review of attachment 148285 [details] [review]: Regarding the decrement operations, I think it'd be cleaner to first check the return value of g_type_info_get_array_length. Then, I'd put an assertion if you think it's needed. Regarding the tests, I think we should have three new tests (for input, output and return), since this patch touches all those parts in the invoke function. And try to remove the lost blank line and whitespace. Thanks!
(In reply to comment #5) > Review of attachment 148285 [details] [review]: > > Regarding the decrement operations, I think it'd be cleaner to first check the > return value of g_type_info_get_array_length. Then, I'd put an assertion if you > think it's needed. Hmm, how do you think it will be cleaner? Seems like it would be noisier that way. > Regarding the tests, I think we should have three new tests (for input, output > and return), since this patch touches all those parts in the invoke function. I'm not sure which tests would those be. Can you give me the names of the new functions you are thinking about? > And try to remove the lost blank line and whitespace. Thanks!
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 148285 [details] [review] [details]: > > > > Regarding the decrement operations, I think it'd be cleaner to first check the > > return value of g_type_info_get_array_length. Then, I'd put an assertion if you > > think it's needed. > > Hmm, how do you think it will be cleaner? Seems like it would be noisier that > way. First, we assert that g_type_info_get_array_length doesn't fail. Then our logic is another thing. > > Regarding the tests, I think we should have three new tests (for input, output > > and return), since this patch touches all those parts in the invoke function. > > I'm not sure which tests would those be. Can you give me the names of the new > functions you are thinking about? test_gi_object_method_array_(in|out|return).
Created attachment 148817 [details] [review] The array field 'length' starts to count from the C arg list, so need to decrement when it's a method
Review of attachment 148817 [details] [review]: Good. Thanks!
Attachment 148817 [details] pushed as 24fa122 - The array field 'length' starts to count from the C arg list, so need to decrement when it's a method