GNOME Bugzilla – Bug 598994
Fix compilation warnings
Last modified: 2010-03-09 16:17:32 UTC
Created attachment 145812 [details] [review] Fix compilation warnings Here a little patch to fix some compilation warnings
Created attachment 145817 [details] [review] Remove some unused variables
Created attachment 145818 [details] [review] Use g_get_current_dir() instead getcwd()
Created attachment 145819 [details] [review] Catch the returned value by write()
Created attachment 145820 [details] [review] Use parenthesis in a expression with "&" operator
Created attachment 145821 [details] [review] Reorder the function so the "lookup_context" variable can't be used uninitialized
Created attachment 145822 [details] [review] Added some default cases and assert if reached
Created attachment 145823 [details] [review] Initialize variable to NULL The function will assert if the variable == NULL after the if's block to assign its value
Comment on attachment 145819 [details] [review] Catch the returned value by write() You probably shouldn't initialize the n_bytes variable to -1. With that fixed, go ahead and commit.
Comment on attachment 145822 [details] [review] Added some default cases and assert if reached Could you still initialize the variables to their default values here? Otherwise compilation with -DG_DISABLE_ASSERT=1 will give you the warnings back. Currently you don't get the warnings because g_assert_not_reached() aborts the program, but that can be disabled, and then they're back.
Comment on attachment 145823 [details] [review] Initialize variable to NULL I think you want to switch (layout) in the dialog_get_pages_per_sheet(dialog)== 2 part here. And don't forget the default case. That should get rid of the need to initialize enum_value to NULL.
Comment on attachment 145820 [details] [review] Use parenthesis in a expression with "&" operator commit 8e70bc988b5fc7589ee949be2f8f768bfd1a173d
Comment on attachment 145819 [details] [review] Catch the returned value by write() committed without initialize n_bytes to -1 commit 33f53e1d0460b26f5dd4713555cc553dbef4847a
Comment on attachment 145818 [details] [review] Use g_get_current_dir() instead getcwd() commit 232fca6e9587041c12d9b81e19d721c661c24207
Comment on attachment 145817 [details] [review] Remove some unused variables commit d3155bb1eaf81a6175dff0347d7e6e146735a4d4
Created attachment 145890 [details] [review] Added some default cases and assert if reached.v2 Initialize variables to default values
Created attachment 145907 [details] [review] Added some default cases and assert if reached.v3
Created attachment 145908 [details] [review] Added some default cases and assert if reached.v4
Comment on attachment 145908 [details] [review] Added some default cases and assert if reached.v4 commit 670e141b3a23c9578dcffe7b04d6a0ff84404bec
Comment on attachment 145821 [details] [review] Reorder the function so the "lookup_context" variable can't be used uninitialized Seems I forget to mark this.
Comment on attachment 145908 [details] [review] Added some default cases and assert if reached.v4 You forgot to set the lower rectangle to 0, too.
Created attachment 145910 [details] [review] Use switch instead if's to get a default value
Comment on attachment 145910 [details] [review] Use switch instead if's to get a default value commit 69773763ee7fbd2c494a5b8c5ff13093715fb788
Comment on attachment 145910 [details] [review] Use switch instead if's to get a default value If you group the switch statements like: case GTK_NUMBER_UP_LAYOUT_LEFT_TO_RIGHT_TOP_TO_BOTTOM: case GTK_NUMBER_UP_LAYOUT_TOP_TO_BOTTOM_LEFT_TO_RIGHT: enum_value = g_enum_get_value (enum_class, GTK_NUMBER_UP_LAYOUT_LEFT_TO_RIGHT_TOP_TO_BOTTOM); break; I'll be perfectly happy.
Comment on attachment 145821 [details] [review] Reorder the function so the "lookup_context" variable can't be used uninitialized commit 69773763ee7fbd2c494a5b8c5ff13093715fb788
Comment on attachment 145910 [details] [review] Use switch instead if's to get a default value commited a version with the switch statements grouped commit 6f6746389c5d08e1534a7eb04f02e00c5b409e50
Created attachment 145915 [details] [review] use G_GSIZE_FORMAT instead %i
Created attachment 145917 [details] [review] Remove an unused function
Created attachment 145918 [details] [review] Cast to guchar*
Comment on attachment 145918 [details] [review] Cast to guchar* If you want to, you can use NULL here instead of (guchar *) "" Looks a bit better and does the same thing.
Comment on attachment 145915 [details] [review] use G_GSIZE_FORMAT instead %i commit d5f1b79fabe73345968b6b1f6035a9ba149adc38
Comment on attachment 145917 [details] [review] Remove an unused function commit d020cb4aa34e786a15a71be72679f37b6164507e
Comment on attachment 145918 [details] [review] Cast to guchar* committed a version using NULL commit c43a3acac3d56d7fb67c996450d3d1ca9f7668e7
Created attachment 145994 [details] [review] Cast to gchar*
Created attachment 145995 [details] [review] Cast to gchar*
Review of attachment 145995 [details] [review]: asdf
Comment on attachment 145995 [details] [review] Cast to gchar* commit 8e945e0e3f1c7df4cb0873cc248181e03dcdc9ce
Created attachment 146175 [details] [review] Cast to gchar* to fix a compilation warning in io-pnm
Created attachment 146176 [details] [review] Cast to gchar* in io-gif
Comment on attachment 146176 [details] [review] Cast to gchar* in io-gif I'm pretty sure this patch is wrong. strcmp() and friends compare the trailing 0 byte to make sure the string is terminated properly, strncmp() doesn't do that. So with this patch the code compares one byte more than previously and this will probably cause it to do bad things. Also, there is no need to use strcmp0() here as both strings are guaranteed to not be NULL, and that's the only reason to use this function.
Comment on attachment 146175 [details] [review] Cast to gchar* to fix a compilation warning in io-pnm commit e21355495a056488fdea98372de2754530b3c86a
Created attachment 146226 [details] [review] Cast to gchar* in io-gif
Created attachment 146227 [details] [review] Cast to gchar* in io-gif
Comment on attachment 146227 [details] [review] Cast to gchar* in io-gif commit 567b23229f78e2ca4d651c3ab60054fda5857d1b
Created attachment 146472 [details] [review] Initialize to false the returned variable
Created attachment 146473 [details] [review] [gdk/x11] Cast to gchar*
Created attachment 146474 [details] [review] [gdk/x11] Fix warning: cast to guint*
Created attachment 146475 [details] [review] [gtk/gtkcolorsel] Fix warning: cast to gchar*
Created attachment 146476 [details] [review] [gtk/gtkmain] Remove statement with no effect
Created attachment 146477 [details] [review] [gtk/gtkobject] Fix warning: cast to gint*
Created attachment 146478 [details] [review] [gtk/gtktext] Fix warning: some casts to gchar*
Created attachment 146479 [details] [review] [gtk/gtkclist] Fix warning: cast to gint8*
Comment on attachment 146472 [details] [review] Initialize to false the returned variable I believe the correct initial return value would be TRUE here, no? Also, could you fix the indentation of that function while you're at it?
Comment on attachment 146474 [details] [review] [gdk/x11] Fix warning: cast to guint* I'd much prefer if you fixed the prototypes of MyEnhancedXkbTranslateKeyCode()and translate_keysym() to take a gint* because those functions treat these arguments as an int anyway. And they're only called from here, too. So we get around the need to cast.
Comment on attachment 146475 [details] [review] [gtk/gtkcolorsel] Fix warning: cast to gchar* Could you add a patch that removes the workaround below where the dash-offset gets made positive? This is fixed since Cairo 0.9.3, and Gtk requires Cairo 1.6 these days.
Comment on attachment 146477 [details] [review] [gtk/gtkobject] Fix warning: cast to gint* This patch doesn't match the description. In fact, it's the previous patch again. If you used git-bz here, please point Owen to this bug.
Comment on attachment 146479 [details] [review] [gtk/gtkclist] Fix warning: cast to gint8* can you instead create do something like gint8 dashes[] = { 4, 4 }; at the beginning of the if and then call gdk_gc_set_dashes (..., dashes, G_N_ELEMENTS (dashes)); That should be clearer.
Comment on attachment 146473 [details] [review] [gdk/x11] Cast to gchar* commit d76287c94b3b6a111eb6bf84315ba9c10ff3eb7f
Created attachment 146544 [details] [review] [gdk/x11/gdkkeys-x11] Fix some compilation warnings
Comment on attachment 146475 [details] [review] [gtk/gtkcolorsel] Fix warning: cast to gchar* commit cbba733b47424cfe44f7a358317db14a406d2374
Comment on attachment 146476 [details] [review] [gtk/gtkmain] Remove statement with no effect commit 07236ee745c06c4dbb210d239bf188cb8dc3550a
Created attachment 146546 [details] [review] [gtk/gtkobject] Fix warning: cast to gint* Sorry, here the correct patch
Comment on attachment 146478 [details] [review] [gtk/gtktext] Fix warning: some casts to gchar* commit 0a9843c1b9d7b58adb2690630d704ad524011a3c
Created attachment 146548 [details] [review] [gtk/gtkclist] Use proper gint8 array variable instead a string
Comment on attachment 146544 [details] [review] [gdk/x11/gdkkeys-x11] Fix some compilation warnings commit 3dbfc08a7a219db290b049a0788db519fcbcb35d
Comment on attachment 146546 [details] [review] [gtk/gtkobject] Fix warning: cast to gint* commit 8763551b6b0035d6ecc762368c838507dccbbbeb
Comment on attachment 146548 [details] [review] [gtk/gtkclist] Use proper gint8 array variable instead a string commit 550796d897380d64675373d763275d69eefbcad1
Comment on attachment 146472 [details] [review] Initialize to false the returned variable commit 141a7dac000a5cc4c103e614291021f0b292660c
Created attachment 146711 [details] [review] Use gpointer
Created attachment 146712 [details] [review] Cast to guint8*
Comment on attachment 146712 [details] [review] Cast to guint8* If this wasn't deprecated code, I'd have asked to check if it wasn't better to use int variables instead of guint for the last two chunks of the patch, but now I won't care. The guint8* casts are perfectly fine in any case.
Comment on attachment 146711 [details] [review] Use gpointer commit 1a02671437369f8b07c7eb0bfaa3de98f41f1bfc
Comment on attachment 146712 [details] [review] Cast to guint8* Comitted a modified version of this patch: I use guint directly instead do a cast commit 8dfac859a15ea90c3015c510247dd3113c14dc26
Created attachment 147236 [details] [review] Cast to guchar*
Created attachment 147237 [details] [review] Cast to gchar*
Created attachment 147238 [details] [review] Use const gchar
Created attachment 147239 [details] [review] Remove unused gtk_text_btree_node_invalidate_downwar() function
Created attachment 147240 [details] [review] Use GINT_TO_POINTER() and GPOINTER_TO_INT()
Created attachment 147241 [details] [review] Use the proper G*_TO_POINTER macro
Created attachment 147242 [details] [review] Initialize variable to 0
Created attachment 147243 [details] [review] Disable test until it works
Created attachment 147244 [details] [review] Initialize variable to NO_MATCH
Created attachment 147245 [details] [review] Use parenthesis in a expression with '&&' and '||'
Comment on attachment 147244 [details] [review] Initialize variable to NO_MATCH First the easy comment: I would omit the first chunk of the patch as it's much clearer to say "return NO_MATCH" than "return result", even if you know that result == NO_MATCH. Now for the second part: I'm pretty sure the logic here is not completely right. If we get a common_prefix and it is_complete_not_unique, you set result = COMPLETED unconditionally, but the old code keeps a value of COMPLETE_BUT_NOT_UNIQUE. I like your idea of unifying the have_result and result variables, but I'm not sure how to best do that in this convoluted code. So I would probably just initialize the variable to NO_MATCH and be done with it.
Comment on attachment 147239 [details] [review] Remove unused gtk_text_btree_node_invalidate_downwar() function You should ask Kris if it's ok to delete the code. If Kris isn't there, just #if 0 the code. Oftentimes people keep code in because they think it might be needed later and then they'll be surprised when its gone.
Comment on attachment 147236 [details] [review] Cast to guchar* commit 73cfcf94953359dc781f30f94eef15593000a2e5
Comment on attachment 147242 [details] [review] Initialize variable to 0 commit e70a46c7e3266e38294ac49e97646d99ab4c83a9
Comment on attachment 147243 [details] [review] Disable test until it works commit c7a5ddf8a7250ae549995508dda894544435f2eb
Comment on attachment 147237 [details] [review] Cast to gchar* commit ef414cc6bf01adf667e036ec1d00e2ad15d98a74
Comment on attachment 147238 [details] [review] Use const gchar commit d8f5ce4b545250a5c8dd50f21d031999b348a8e3
Comment on attachment 147245 [details] [review] Use parenthesis in a expression with '&&' and '||' commit 8831a4490264d9ac6ecac8014a066fabd2fbdc6e
Comment on attachment 147240 [details] [review] Use GINT_TO_POINTER() and GPOINTER_TO_INT() commit 20f123531a70bce7c6b96b788aa22d7d1551d392
Comment on attachment 147244 [details] [review] Initialize variable to NO_MATCH Committed a version of this patch only with this change, as you requested: - CommonPrefixResult result; + CommonPrefixResult result = NO_MATCH; commit 1919f55647007d1ca1f9fd38b5108e0adb54ea30
Comment on attachment 147241 [details] [review] Use the proper G*_TO_POINTER macro if you remove the (void *) casts, it'll be perfect.
Comment on attachment 147241 [details] [review] Use the proper G*_TO_POINTER macro commit f1f6d76078dccd1c7d7c305a45c270451ead6612
Created attachment 147317 [details] [review] Use G_POINTER_TO_UINT
Review of attachment 147317 [details] [review]: Dear Splinter, please accept empty reviews when one changes the patch status. Thank you.
Comment on attachment 147239 [details] [review] Remove unused gtk_text_btree_node_invalidate_downwar() function Use #if 0 instead remove the code commit 3143a344a76ebaf9e931c00efd8b797074d56542
Comment on attachment 147317 [details] [review] Use G_POINTER_TO_UINT commit 78151dc7d41069e876fe165527e68c238ef7fd4a
Created attachment 147322 [details] [review] Use G_GNUC_UNUSED macro
Review of attachment 147322 [details] [review]: looks good
Comment on attachment 147322 [details] [review] Use G_GNUC_UNUSED macro commit 21f6e1841ae1546cf109ed83ff0fbb705212b29d
Created attachment 147328 [details] [review] Use a variable to store the return value
Review of attachment 147328 [details] [review]: r=me
Comment on attachment 147328 [details] [review] Use a variable to store the return value commit cb301e6dbbe8c4017c7022995976b577796cf903
Created attachment 148881 [details] [review] Remove useless line
Created attachment 148882 [details] [review] Use {} in empty if..else blocks
Comment on attachment 148882 [details] [review] Use {} in empty if..else blocks Could you close this bug when you've committed the fixes? This bug is getting crowded. Feel free to open a new one for the next sets of compiler warnings and cc me there. (It will increase our bugzilla score, too!)
Comment on attachment 148881 [details] [review] Remove useless line commit f35c581de43532c493439c05ad6c1b19942e000e
Comment on attachment 148882 [details] [review] Use {} in empty if..else blocks commit 62bfbe91d89bf7c4c91bd4384649c01dccde2b74
(In reply to comment #107) > (From update of attachment 148882 [details] [review]) > Could you close this bug when you've committed the fixes? This bug is getting > crowded. > Feel free to open a new one for the next sets of compiler warnings and cc me > there. > (It will increase our bugzilla score, too!) Sure, I'll open a new bug If I catch more compilation warnings, but I'd like to keep this open until the gtk/gtkimcontextsimpleseqs.h downcasting problem was solved. As always, thank you again for your reviews ;)
As bug #601234 is now fixed, we can close this bug because there is not compilation warnings in GTK+ now :)