GNOME Bugzilla – Bug 710076
Fix lots of warnings in libcroco 0.6.8
Last modified: 2015-11-01 10:19:52 UTC
GCC complains a lot about missing casts, unused variables, etc.
Created attachment 257207 [details] [review] 0001-proper-pointer-derivation
Created attachment 257208 [details] [review] 0002-cast-for-strdup.all
Created attachment 257209 [details] [review] 0003-fix-unused-vars
Created attachment 257210 [details] [review] 0004-fix-type
Created attachment 257211 [details] [review] 0005-fix-casts
Created attachment 257212 [details] [review] 0006-fix-casts2
Created attachment 257213 [details] [review] 0007-fix-type2
Created attachment 257214 [details] [review] 0008-fix-type-and-casts
Created attachment 257215 [details] [review] 0009-dont-shadow
Created attachment 257216 [details] [review] 0010-more-casts
Created attachment 257217 [details] [review] 0011-more-casts2
Created attachment 257218 [details] [review] 0012-more-casts3
Created attachment 257219 [details] [review] 0013-more-casts4
Created attachment 257220 [details] [review] 0020-more-casts10
Created attachment 257221 [details] [review] 0021-more-casts11
Created attachment 257222 [details] [review] 0022-more-casts12
Created attachment 257223 [details] [review] 0023-even-more-casts
Created attachment 257224 [details] [review] 0024-even-more-casts2
Created attachment 257225 [details] [review] 0025-even-more-casts3
Created attachment 257226 [details] [review] 0026-even-more-casts4
Created attachment 257227 [details] [review] 0027-even-more-casts5
Created attachment 257228 [details] [review] 0028-even-more-casts6
Created attachment 257229 [details] [review] 0029-even-more-casts7
Created attachment 257230 [details] [review] 0030-even-more-casts8
Comment on attachment 257208 [details] [review] 0002-cast-for-strdup.all Should note in the commit msg that this is also both a correctness fix and a mem leak fix!
Comment on attachment 257213 [details] [review] 0007-fix-type2 This doesn't look right, you're assigning -1 to an unsigned type.
Comment on attachment 257217 [details] [review] 0011-more-casts2 But please consolidate all the patches that just fix guchar/gchar into one commit.
Comment on attachment 257222 [details] [review] 0022-more-casts12 - g_return_val_if_fail (a_predefined >= PREDEFINED_ABSOLUTE_FONT_SIZE - && a_predefined < NB_FONT_SIZE_TYPE, + g_return_val_if_fail (a_predefined >= FONT_SIZE_XX_SMALL + && a_predefined < NB_PREDEFINED_ABSOLUTE_FONT_SIZES, CR_BAD_PARAM_ERROR) ; What's this part about ? Separate it from the casts.
(In reply to comment #25) > (From update of attachment 257208 [details] [review]) > Should note in the commit msg that this is also both a correctness fix and a > mem leak fix! Yes (In reply to comment #26) > (From update of attachment 257213 [details] [review]) > This doesn't look right, you're assigning -1 to an unsigned type. Yeah, -1 in gulong looks wrong. Sadly, gcc didn't warn me about _that_ when i did this. However, i did look at the code and it turns out that this is actually correct (almost). The code uses that glong to pass the number of spaces that should be consumed. "-1" means "consume all". What the comment doesn't tell you is that the function takes gulong, and -1 gets re-interpreted as very-large-number in gulong, and the code in that function checks for number-of-consumed-spaces < very-large-number, which is how it works. (In reply to comment #27) > (From update of attachment 257217 [details] [review]) > But please consolidate all the patches that just fix guchar/gchar into one > commit. I've been fixing this on file-by-file basis, and worked against a release tarball, not a git clone, so the form you're asking does not appear here naturally (i will actually have to get a git clone, go through the patches, pick hunks that only deal with guchar/char, make them into a separate commit, then rebase everything on top of that). Do you really need that? (In reply to comment #28) > (From update of attachment 257222 [details] [review]) > - g_return_val_if_fail (a_predefined >= PREDEFINED_ABSOLUTE_FONT_SIZE > - && a_predefined < NB_FONT_SIZE_TYPE, > + g_return_val_if_fail (a_predefined >= FONT_SIZE_XX_SMALL > + && a_predefined < > NB_PREDEFINED_ABSOLUTE_FONT_SIZES, > CR_BAD_PARAM_ERROR) ; > > What's this part about ? Separate it from the casts. For "separate it" - see above. The part is about wrong enums being used. a_predefined is a CRPredefinedAbsoluteFontSize, not a CRFontSizeType, and the sanity check here should use appropriate values.
Separating is really easy: git add -p . Since almost all of the patches are those gchar/guchar casts, you can just commit the few extra bits separately, then squash all the casts into one commit.
LRN give it a try at this again?
libcroco haven't had a new release for almost 2 years now, and practically no commits. That lessens my motivation in pushing things upstream - why do that, if anything i contribute is not going to be released? I can still apply patches in my own builds (i do!), and i don't care how they look when i do that.
There are no releases because there were no commits. If these fixes get pushed I am pretty sure we can quickly make a new minor release.
Created attachment 282511 [details] [review] Fix unused and set-but-unused vars warnings
Created attachment 282512 [details] [review] gchar/guchar types and casting
Created attachment 282513 [details] [review] char/xmlChar casting
Created attachment 282514 [details] [review] Fix the unused arguments warnings
Created attachment 282515 [details] [review] Derive pointer to have appropriate type
Created attachment 282516 [details] [review] Fix improper use of g_ascii_strup()
Created attachment 282517 [details] [review] Use the right values for checking CRPredefinedAbsoluteFontSize
Created attachment 282518 [details] [review] Use unsigned types where appropriate
Created attachment 282519 [details] [review] Fix cr_input_consume_white_spaces to behave as documented
Created attachment 282520 [details] [review] Ensure PEEK_NEXT_CHAR does not clash with existing variables
Created attachment 282521 [details] [review] Remove shadowing in cr_statement_media_rule_to_string()
Created attachment 282522 [details] [review] Fix cr_tknzr_consume_chars() to set *a_nb_char before returning
Created attachment 282523 [details] [review] Remove shadowing from cr_tknzr_get_next_token()
Review of attachment 282523 [details] [review]: this is super trivial and correct
Review of attachment 282522 [details] [review]: this looks a bit strange: while the patch is correct and shuts up the warning, why does the library uses different types for the same thing in different part of the api? is any of those two functions private so that it can be changed to use the same type as the other?
Review of attachment 282521 [details] [review]: obviously correct
Review of attachment 282520 [details] [review]: makes sense. I would use underscores for local macro variables, but I guess it depends on the convention used throughout the codebase.
Review of attachment 282519 [details] [review]: the patch itself makes sense... did you check the callers? did this work because they are all ignoring that value? can NULL be passed in? if yes, we need to check before assigning
Review of attachment 282518 [details] [review]: looks good
Review of attachment 282517 [details] [review]: looks correct. The check are actually less strict... I wonder if nobody ever tried to call that function with FONT_SIZE_XX_LARGE
Review of attachment 282516 [details] [review]: This is clearly correct "locally", but I wonder if there are some other bugs that work relying on the fact that the string is not actually uppercased in the current code. I'll leave this one to someone more familiar with the code
Review of attachment 282514 [details] [review]: looks good. if possible fix the trivial formatting pointed out below ::: src/cr-om-parser.c @@ -387,2 +387,4 @@ CRStatement *stmt = NULL; + (void) a_page; + (void) a_pseudo_page; leave an empty line @@ -389,1 +391,1 @@ g_return_if_fail (a_this); while at it add an empty line after g_return_if_fail @@ -450,2 +452,3 @@ CRStatement *stmts = NULL; + (void) a_media_list; same @@ -489,2 +492,3 @@ GList *media_list = NULL ; + (void) a_uri_default_ns; same @@ -562,2 +566,3 @@ ParsingContext **ctxtptr = NULL; + (void) a_selector_list; same ::: tests/test2-main.c @@ -112,2 +112,3 @@ CRParsingLocation *a_location) { + (void) a_uri; leave an empty line ::: tests/test3-main.c @@ -110,2 +110,3 @@ g_return_if_fail (a_handler); + (void) a_uri; leave an empty line
Review of attachment 282513 [details] [review]: this looks correct. I hate this about libxml :-) the casting style is a bit not consistent (sometime "char *", sometime "char*") but I see that it was like that before your patch so I assume you followed the surrounding code
Review of attachment 282511 [details] [review]: this is trickier than just warnings: it looks like real bugs pointed out by warnings. I'll leave review to chpe for now
Review of attachment 282512 [details] [review]: this looks correct (I admit I have not checked each cast one by one). but as far as I can see chpe already okayed this in the old version he just asked to consolidate things in a single patch
Christian: for the record LRN is doing a wonderful job of pushing upstream a huge load of patches he had in a separate repository to get our stack compiling and working properly on Windows. Nacho and me are helping him with reviews etc and since you seem to be away I stepped in to accept at least the trivial parts of this patchset, I hope that is ok.
Attachment 282521 [details] pushed as 4a65fa0 - Remove shadowing in cr_statement_media_rule_to_string() Attachment 282523 [details] pushed as 2e56f96 - Remove shadowing from cr_tknzr_get_next_token()
Attachment 282512 [details] pushed as d27114c - gchar/guchar types and casting Attachment 282513 [details] pushed as b77da88 - char/xmlChar casting Attachment 282517 [details] pushed as a7df9d2 - Use the right values for checking CRPredefinedAbsoluteFontSize Attachment 282518 [details] pushed as 0284435 - Use unsigned types where appropriate Attachment 282520 [details] pushed as 6824af5 - Ensure PEEK_NEXT_CHAR does not clash with existing variables
Comment on attachment 282514 [details] [review] Fix the unused arguments warnings Attachment 282514 [details] pushed as 06e15e2 - Fix the unused arguments warnings
(In reply to comment #48) > Review of attachment 282522 [details] [review]: > > this looks a bit strange: while the patch is correct and shuts up the warning, > why does the library uses different types for the same thing in different part > of the api? is any of those two functions private so that it can be changed to > use the same type as the other? No idea. That said, i'm a fan of strong typing, so if there are two distinct types for two distinct things, i'd say we keep them distinct, even if one type could be used for values of both kinds. (In reply to comment #51) > Review of attachment 282519 [details] [review]: > > the patch itself makes sense... did you check the callers? did this work > because they are all ignoring that value? can NULL be passed in? if yes, we > need to check before assigning cr_input_consume_white_spaces() does not document that the last argument can optionally be NULL. Also, it documents that argument as In/Out, meaning that it kind of must be non-NULL (it's the way you tell it how many spaces to consume), always. cr_input_consume_white_spaces() is only called once in libcroco - in cr-tknzr.c, from cr_tknzr_try_to_skip_spaces(). That call does specify the last argument, it's a pointer to -1 (casted to gulong, now that the use-unsigned patch is in). And that's it. Unless there are clients that call this function (it doesn't seem to be private). That said, googling gave me no results.
(In reply to comment #59) > Christian: for the record LRN is doing a wonderful job of pushing upstream a > huge load of patches he had in a separate repository to get our stack compiling > and working properly on Windows. > Nacho and me are helping him with reviews etc and since you seem to be away I > stepped in to accept at least the trivial parts of this patchset, I hope that > is ok. That's fine. Just mark everything a-c-n that you think is ok. You can even take over libcroco, if you like :-)
attachment 282515 [details] [review] is still valid attachment 282516 [details] [review] is still valid attachment 282519 [details] [review] is still valid attachment 282522 [details] [review] is still valid attachment 282511 [details] [review] might be incorrect, i'll upload different patches for this issue
Created attachment 314562 [details] [review] Fix unused status variable in cr_utils_ucs1_to_utf8() * Return status instead (initialized to CR_OK) instead of an explicit CR_OK * Remove redundant check for *a_in_len < 1 (equivalent to *a_in_len == 0) and remove now-unused end label
Created attachment 314563 [details] [review] Fix unused status variable in cr_utils_utf8_to_ucs1() * Don't set status to CR_OK, it's initialized to that value * Return status
Created attachment 314564 [details] [review] Fix unused status variable in set_prop_position_from_value() * Return status
Created attachment 314565 [details] [review] Fix unused status variable in evaluate_selectors() * Add an end label to go to on error * Clean up xml_doc at the end * Fix a miscleaning of xpath_object * Unref sheets after they are given to a cascade * Return status at the end
Created attachment 314566 [details] [review] Fix unused status variable in main()
Review of attachment 314566 [details] [review]: See the comment. Feel free to fix it and push it. ::: csslint/csslint.c @@ +1007,3 @@ } + return (status == CR_OK) ? 0 : -2; why -2? let's return -1.
Review of attachment 314565 [details] [review]: See the minor comments. Apart from that it looks good. Feel free to push it after the changes ::: csslint/csslint.c @@ +403,3 @@ } + end: usually I prefer labels to not have indentation. @@ +423,3 @@ + if (xml_doc) { + xmlFreeDoc (xml_doc); + xml_doc = NULL; why setting to NULL if it will not be used anymore? Same for the other vars right above
Review of attachment 314564 [details] [review]: Looks good
Review of attachment 314563 [details] [review]: Looks good.
Review of attachment 314562 [details] [review]: Looks good.
Review of attachment 282515 [details] [review]: See the comment and feel free to push it. ::: src/cr-enc-handler.c @@ +91,3 @@ if (gv_default_enc_handlers[i].encoding == a_enc) { return (CREncHandler *) + & gv_default_enc_handlers[i]; Looks good. I'd put it int he same line as return though
(In reply to Ignacio Casal Quinteiro (nacho) from comment #71) > Review of attachment 314566 [details] [review] [review]: > > See the comment. Feel free to fix it and push it. > > ::: csslint/csslint.c > @@ +1007,3 @@ > } > > + return (status == CR_OK) ? 0 : -2; > > why -2? let's return -1. Because -1 means something else (bad arguments?). (In reply to Ignacio Casal Quinteiro (nacho) from comment #72) > Review of attachment 314565 [details] [review] [review]: > > ::: csslint/csslint.c > @@ +403,3 @@ > } > > + end: > > usually I prefer labels to not have indentation. So do i, but libcroco code generally indents labels for some reason, so i kept that (although maybe the amount of indentation is wrong). > > @@ +423,3 @@ > + if (xml_doc) { > + xmlFreeDoc (xml_doc); > + xml_doc = NULL; > > why setting to NULL if it will not be used anymore? Same for the other vars > right above Again, this is what libcroco was already doing, so i kept doing that. Most likely it's an insurance in case anything gets done with the pointer between the destruction and the return statement (it's better to try to dereference NULL and fail instead of dereferencing a pointer to a freed memory which might or might not contain valid data still).
Review of attachment 282519 [details] [review]: Let's push it leaving an empty line before assinging it.
Review of attachment 282516 [details] [review]: Let's push it now that we are in the early cycle. If something it can be reverted
Review of attachment 282522 [details] [review]: In relation to pbor's comment. It is a change from gulong to glong, I think it is a typo but too late to change since it is public api :(. Fix the minor issues and please push it. ::: src/cr-tknzr.c @@ +1915,3 @@ + status = cr_input_consume_chars (PRIVATE (a_this)->input, + a_char, &consumed); fix the indentation here @@ +1916,3 @@ + status = cr_input_consume_chars (PRIVATE (a_this)->input, + a_char, &consumed); + *a_nb_char = (glong ) consumed; remove useless space before )
Attachment 282515 [details] pushed as 3f94a4f - Derive pointer to have appropriate type Attachment 282516 [details] pushed as 0ab22e8 - Fix improper use of g_ascii_strup() Attachment 282519 [details] pushed as 5fd3023 - Fix cr_input_consume_white_spaces to behave as documented Attachment 282522 [details] pushed as bc9b2c3 - Fix cr_tknzr_consume_chars() to set *a_nb_char before returning Attachment 314562 [details] pushed as ac8029a - Fix unused status variable in cr_utils_ucs1_to_utf8() Attachment 314563 [details] pushed as 6bb010a - Fix unused status variable in cr_utils_utf8_to_ucs1() Attachment 314564 [details] pushed as d03fa21 - Fix unused status variable in set_prop_position_from_value() Attachment 314565 [details] pushed as f09307d - Fix unused status variable in evaluate_selectors() Attachment 314566 [details] pushed as 956078a - Fix unused status variable in main()