On Wed, Jun 6, 2012 at 12:57 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > On Tue, Jun 05, 2012 at 05:36:38PM +0300, Zeeshan Ali (Khattak) wrote: >> On Tue, Jun 5, 2012 at 10:54 AM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: >> > On Mon, Jun 04, 2012 at 08:38:14PM +0300, Zeeshan Ali (Khattak) wrote: >> >> On Mon, Jun 4, 2012 at 7:26 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: >> >> > On Mon, Jun 04, 2012 at 06:44:21PM +0300, Zeeshan Ali (Khattak) wrote: >> >> >> From: "Zeeshan Ali (Khattak)" <zeeshanak@xxxxxxxxx> >> >> >> >> >> >> This function was adding the second list elements to new list first so >> >> >> the order of elements in the new list was contrary to what user will >> >> >> expect of this function (and all wrapper/using functions). >> >> > >> >> > This change makes sense to me, but since you looked at this code to fix a >> >> > Boxes bug, you should explain what behaviour you were seeing without this >> >> > patch, what this patch change, and why it's better now (or something in >> >> > that spirit ;) >> >> >> >> If the change makes sense anyways, I don't see the need to waste time >> >> on justifying it. >> > >> > The very fact that I needed to first guess and then explain where this >> > patch is coming from in the paragraph below should be proof enough that >> > explaining why you had to look into that code was required to achieve a >> > proper review of said code. >> > By doing that, you're not only helping the reviewers, but also your future >> > self, if you find a bug in this code 6 months from now, having a log >> > explaining why you made this change a long time ago can be a tremendous >> > help to make sure you are not introducing regressions. >> > That said, after the discussion below saying that it's not an actual fix >> > for the bug you were seeing, this explanation is less needed in this patch, >> > but will be useful in the patch with the true fix. >> >> So this patch can go in already? > > Well, it would be bad if this patch went in, and became a good enough work > around for Boxes, and if the root cause never gets fixed... I don't at all understand the logic of keeping a bug around even though there is a fix for it just because it exposes another (bigger bug). Especially if the other bug is hard to solve. The solution for the human memory problem would to file a bug about it and/or write it down somewhere. So for this patch, all I need to know is if there is any problem with it still to be addressed? -- Regards, Zeeshan Ali (Khattak) FSF member#5124
