After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 602640 - Array lengths are bogus at times
Array lengths are bogus at times
Status: RESOLVED FIXED
Product: pygi
Classification: Deprecated
Component: general
unspecified
Other All
: Normal normal
: 0.6
Assigned To: Tomeu Vizoso
pygi-maint
Depends on:
Blocks:
 
 
Reported: 2009-11-22 15:52 UTC by Tomeu Vizoso
Modified: 2009-12-22 17:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
The array field 'length' starts to count from the C arg list, so need to decrement when it's a method (8.40 KB, patch)
2009-11-22 15:52 UTC, Tomeu Vizoso
needs-work Details | Review
The array field 'length' starts to count from the C arg list, so need to decrement when it's a method (6.95 KB, patch)
2009-11-22 17:47 UTC, Tomeu Vizoso
needs-work Details | Review
The array field 'length' starts to count from the C arg list, so need to decrement when it's a method (8.10 KB, patch)
2009-12-01 11:42 UTC, Tomeu Vizoso
committed Details | Review

Description Tomeu Vizoso 2009-11-22 15:52:30 UTC
The array field 'length' starts to count from the C arg list, so need to decrement when it's a method
Comment 1 Tomeu Vizoso 2009-11-22 15:52:32 UTC
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
Comment 2 Simon van der Linden 2009-11-22 16:12:47 UTC
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!
Comment 3 Tomeu Vizoso 2009-11-22 17:47:40 UTC
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
Comment 4 Tomeu Vizoso 2009-11-22 17:48:25 UTC
(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.
Comment 5 Simon van der Linden 2009-11-23 22:15:10 UTC
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!
Comment 6 Tomeu Vizoso 2009-11-24 12:47:15 UTC
(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!
Comment 7 Simon van der Linden 2009-11-30 20:56:19 UTC
(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).
Comment 8 Tomeu Vizoso 2009-12-01 11:42:52 UTC
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
Comment 9 Simon van der Linden 2009-12-22 09:34:37 UTC
Review of attachment 148817 [details] [review]:

Good. Thanks!
Comment 10 Tomeu Vizoso 2009-12-22 17:00:24 UTC
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