On Sun, 31 Aug 2008, Wolfgang Denk wrote: > Dear Guennadi Liakhovetski, > > In message <[EMAIL PROTECTED]> you wrote: > > > > It's not purely stylistic. For example, previously the code had fd and > > fdr, curdev and otherdev. It used one erase struct for both main and > > redundant copies, thus they had to initialise it multiple times to one or > > another version. I separated it into two erase_current and erase_target > > thus removing the need for multiple initialisation. I think, having > > Ah! So there was a functional change (but - what was this needed > for?), which esceaped me because it was buried across all those > variable renamings. > > > dev_target, erase_target, fd_target vs. dev_current, erase_current and > > fd_current is also a readability improvement. > > > > > I reject this patch. > > > > Please, reconsider. > > I still reject it, at least as is. Whether you call the variables > curdev and otherdev versus dev_current and dev_target makes no > significant change to me, except that the new names are longer, more > difficult to type and to read. And any functional changes become > completely invisible among all the renaming. This makes such patches > unacceptable to me. > > It is important that you can actually SEE what a patch is changing. > With your patches, this is not the case. You change everything, and > the significant modifications become invisible.
Well, as I worked on this patch series, it was pretty difficult to me to recognise which of the two environment copies the current code was dealing with. So, to help myself and any future developers, that will work with this code, I decided to make this distinction clearer and more consistent. Sorry, but to me it wasn't obvious, that fd was referring to devcur, and fdr to devother - just from the naming. Whereas using fd_current, dev_current and fd_target, dev_target makes it clearer, IMHO. Yes, this is longer to type, and produces longer lines. But I am prepared to pay this price:-) As for the "functional" change - introducing an additional erase variable to avoid having to reuse and reinitialise one several times - it is a related change, it also separates handling of the two environment copies. So, I think, they belong to one patch. Would it suffice to change the patch description for this patch to be accepted, or do you still want this patch to be dropped / changes? We could use fdcur, fdtrg, devcur, devtrg, erasecur, erasetrg to save the typing, but, personally, I find dev_current easier to read. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [EMAIL PROTECTED] _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
