GNOME Bugzilla – Bug 439122
Compilation warning in evolution's calendar folder
Last modified: 2013-09-13 00:54:46 UTC
Please describe the problem: to follow, a patch to fix most compilation warnings in evolution/calendar Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 88331 [details] [review] evolution-calendar-warning.patch left is a cast I couldn't yet find a way to sanitize.
Looks fine Gilles. Im sure that it would fix some bugs as well (return values from g_list* weren't handled) Thanks
Committed to Subversion trunk (revision 33625).
Note that this patch causes the gnome-pilot conduits to crash. I'm going to back out the patch for the conduit files in bug 201167, as I'm currently trying to combine a few patches into one for the conduits. I'll also add a comment to allow future developers know not to touch it. The specific problem is that ctxt->locals should not be modified by the return of g_slist_prepend() in the calendar-conduit.c, todo-conduit.c, and memo-conduit.c files. We're not losing anything - the calling gnome-pilot function handles it. The change added by this bug fix screws up the iteration that's happening and we end up with corrupt/null pointers when the conduit is destroyed at the end of a sync. Specifically, todoconduit_destroy_record() crashes when trying to call free_ToDo(), and g_object_unref() complains about a bad reference. The same happens for the other conduits.
I think the fix in calls of g_object_add_weak_pointer is wrong, because as a second parameter would be used a pointer which should be set to NULL after the object is freed. In a patch there is passed an address into local variable, so it may not work as expected. Even I don't like this kind of coding, try to write it in this way: g_object_add_weak_pointer (G_OBJECT (priv->vbox), (gpointer *) (void *) &(priv->vbox)); I also noticed that the patch for unpack_Appointment call can be done in the previous (two lines above) call too. To Nathan, I didn't get the point of your comment, why it should not be modified? It looks very strange, the fix do (from my point of view) exactly what is supposed to do (in this part).
Milan is right, the second argument to g_object_add_weak_pointer() must be the address of the pointer that will be set to NULL when the object is finalized. There's two ways to clean up strict aliasing warnings: 1) Declare the "vbox" member of EItipControlPrivate to be a gpointer rather than a GtkWidget. 2) Define a vbox as a union in the EItipControlPrivate struct: union { gpointer pointer; GtkWidget *widget; } vbox; The go through the file and change all usage of priv->vbox to priv->vbox.widget where it should be used as a object or widget. The g_object_add_weak_pointer() call would then look like: g_object_add_weak_pointer ( G_OBJECT (priv->vbox.widget), &priv->vbox.pointer); I personally prefer the second approach even though it's much more verbose, but either way should do it. More info: http://www.cellperformance.com/mike_acton/2006/06/understanding_strict_aliasing.html
Reopening to address the above comments.
Created attachment 94623 [details] evo_build.log I forget to mention, it introduce other/new warnings on my 64bit machine/system, like this: e-tasks.c: In function 'e_tasks_open_task_id': e-tasks.c:1462: warning: cast from pointer to integer of different size We had a chat on IRC so I'm attaching here file, which contains errors from a build of whole Evolution on my machine. I've one for eds too, if you are interested.
Created attachment 94982 [details] [review] evolution-calendar-weak-refs.patch ok this should address the incorrect assignation and the potential bug that Milan explained.
Second one should be priv->hbox, I bet ;) Also notice, original patch has weak pointer change in insert_ok too. Please do ChangeLog entry too, thanks.
Created attachment 95044 [details] [review] evolution-calendar-weak-refs.patch ok, I think that's it this time.
Looks good to commit from my side.
The patch will fix the behavior but it still breaks strict aliasing rules. See comment #6 for how to fix this properly. I'll approve this in order to fix the bug, but we'll have to revisit the strict aliasing issue at some point.
commited and removed the warning suppression part as discussed with Matthew and Milan on irc.
Created attachment 96450 [details] [review] proposed evo patch for evolution; Here's a fix for rest compiler warnings in calendar subdirectory, which I saw on my x86_64 system/machine. I also fixed some leaks there.
Never comment something like this. + gdouble width/*, height*/; Also foo { /*comment */ It sort of spoils the readability of the code. I know there are some. But lets not add any new.
Created attachment 96618 [details] [review] proposed evo patch ][ for evolution; I'm sorry I did this. I removed that /*, height*/ comment and moved those /* to fix strict-aliasing */ comments above unions. I hope it's better now. I didn't change anything else there (except of the date in ChangeLog).
Milan, I dont see any obvious fix. Havent tested it. Just test/compile and commit.
Removed part of * gui/e-itip-control.c * gui/dialogs/alarm-dialog.c * gui/e-day-view-layout.c * gui/e-day-view-layout.h * gui/print.c because it is there already. So the only thing which kept here is the fix for bug #270605. So I obsolete my patch and attach improved one to that bug.
I changed my mind, I keep that patch to Suman there. Closing this as fixed, because all has been committed regarding to calendar directory. (If I didn't overlooked something.)