GNOME Bugzilla – Bug 781955
Segmentation fault when week start in past month
Last modified: 2017-05-09 20:03:19 UTC
Created attachment 350751 [details] [review] Patch April: 30 01 02 03 04 05 06 Drag and drop of multiday event in this week, you will get a Segmentation Fault. Small bug
Created attachment 350766 [details] [review] Patch 2
Review of attachment 350751 [details] [review]: Nice catch, almost there! There are 2 problems with this patch: 1. It causes a memory leak. GDateTime is an immutable object, and every operation creates a new object. You should unref() the previous datetime after adding the days. 2. The code does not follow the GNU C Coding Style About the commit itself, please follow the gudelines at https://wiki.gnome.org/Newcomers/SubmitPatch to write the commit message. And also, looks like your author name is broken (all I can see is < ²`‰U> here)
Review of attachment 350766 [details] [review]: Same of the first patch.
Created attachment 350910 [details] [review] Fixed patch I hope this is right patch
Created attachment 350911 [details] [review] Fixed patch2 Obsolete other patches, and deleted unnecessary newline in last patch
Review of attachment 350911 [details] [review]: Almost there! Besides the code comments below, I believe the title is wrong in "week-header: week-header:". Should be only one "week-header:" prefix, right? ::: src/views/gcal-week-header.c @@ +1386,3 @@ week_start = get_start_of_week (self->active_date); + GDateTime *tmp_dt; This is wrongly placed. It should be at the beginning of the function, with the other declarations. Besides that, you can use the g_autoptr() facility. @@ +1412,3 @@ + dnd_date = g_date_time_add_days (tmp_dt, drop_cell); + g_clear_pointer (&tmp_dt, g_date_time_unref); By using g_autoptr(), you won't need to clear it.
Created attachment 350994 [details] [review] PatchV4 Thanks for review, You're right, Because of bug in gitg (https://bugzilla.gnome.org/show_bug.cgi?id=781263), i have to write patch by hand. It corrupts author info and commit messages. I fixed commit title. Also now, using g_autoptr.
Review of attachment 350994 [details] [review]: Sorry for the back and forth. I forgot to ask you one thing: because we're using g_autoptr(), you also have to initialize tmp_dt to NULL in the beginning of the function, otherwise we might crash if we enter the "if (!GCAL_IS_EVENT_WIDGET (event_widget))" condition. Also, looks like the other patch (for week grid) was marked as obsolete. Was that on purpose? Are you going to upload another patch about that?
Created attachment 351018 [details] [review] PatchV5 Thanks for review, I never use glib library. I'm learning with these bugs. These "back and forth" is good for me. With patch, initialized tmp_dt to NULL. I don't know "week grid patch"? I think it's not me or i misunderstood? :)
(In reply to kestanegurgen from comment #9) > I don't know "week grid patch"? I think it's not me or i misunderstood? :) Er, you're right, there never had a week grid patch. Sorry for the noise!
Review of attachment 351018 [details] [review]: The patch looks mostly good (except one very minor nitpick I commented below), but it appearently doesn't apply cleanly on top of git master. Could you please rebase this patch? After that, it'll be good to go. ::: src/views/gcal-week-header.c @@ +1354,3 @@ g_autoptr (GDateTime) dnd_date; g_autoptr (GDateTime) new_end; + g_autoptr (GDateTime) tmp_dt = NULL; Please initialize this tmp_dt a few lines below, together with week_start, dnd_date and new_end.
Created attachment 351238 [details] [review] PatchV6 Sorry for late response I added new patch, rebase master and initilize tmp_dt with others.
Review of attachment 351238 [details] [review]: Perfect. Thanks for the patch!
Thanks for the patch!