Date:	Mon, 30 Apr 2012 09:59:40 +0200
From:	Borislav Petkov <bp@...64.org>
To:	Mauro Carvalho Chehab <mchehab@...hat.com>
Cc:	Linux Edac Mailing List <linux-edac@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Aristeu Rozanski <arozansk@...hat.com>,
	Doug Thompson <norsk5@...oo.com>,
	Mark Gross <mark.gross@...el.com>,
	Jason Uhlenkott <juhlenko@...mai.com>,
	Tim Small <tim@...tersideup.com>,
	Ranganathan Desikan <ravi@...ztechnologies.com>,
	"Arvind R." <arvino55@...il.com>, Olof Johansson <olof@...om.net>,
	Egor Martovetsky <egor@...emi.com>,
	Chris Metcalf <cmetcalf@...era.com>,
	Michal Marek <mmarek@...e.cz>, Jiri Kosina <jkosina@...e.cz>,
	Joe Perches <joe@...ches.com>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Hitoshi Mitake <h.mitake@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Niklas Söderlund 
	<niklas.soderlund@...csson.com>,
	Shaohui Xie <Shaohui.Xie@...escale.com>,
	Josh Boyer <jwboyer@...il.com>, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH EDACv16 1/2] edac: Change internal representation to work
 with layers

On Sun, Apr 29, 2012 at 11:16:53AM -0300, Mauro Carvalho Chehab wrote:
> > Hey, are you looking at compiled code or at source code? Because I'm
> > looking at source code, and it is a pretty safe bet the majority of the
> > people here do that too.
> 
> What I said is that, from source code POV, a code where the loop variables are
> initialized just before the loop is easier to read it when the initialization
> of those vars are on another part of the code.
> 
> That's basically why the "for" syntax starts with a var initialization clause.
> 
> The tot_dimms & friends are loop vars: their value is calculated within the loop.
> 
> At the object code, this won't bring any difference.
> 
> > 
> >> it, either by using registers for those vars or by moving the initialization
> >> to the top of the function.
> >>
> >> This function is too complex, so it is better to initialize those vars
> >> just before the loops that are calculating those totals.
> > 
> > Simply initialize those variables at declaration time and that's it.
> > Initializing them before the loop doesn't make the function less complex
> > - splitting it and sanitizing it does.
> 
> Initializing loop-calculated vars just before the loop makes the code easier
> to read, and may avoid issues that might happen during code lifecycle.

This is getting ridiculous: the variable declaration and initialization
are on the same screen as the loop (unless one uses a screen which can
only show less than 40ish lines).

So the argument about making the code easier to read is bogus.

This function is already cluttered with a lot of crap, and is very large
so adding more lines which can simply be stashed away at declaration
time is better readability.

Besides, every modern editor can jump to the declaration of a local
variable so that the user can see to what it is initialized to.

> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
> +                                    unsigned n_layers,
> +                                    struct edac_mc_layer *layers,
> +                                    bool rev_order,
> +                                    unsigned sz_pvt)
>  {
>       void *ptr = NULL;
>       struct mem_ctl_info *mci;
> -     struct csrow_info *csi, *csrow;
> +     struct edac_mc_layer *layer;
> +     struct csrow_info *csi, *csr;
>       struct rank_info *chi, *chp, *chan;
>       struct dimm_info *dimm;
> +     u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
>       void *pvt;
> -     unsigned size;
> -     int row, chn;
> +     unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
> +     unsigned tot_csrows, tot_channels, tot_errcount = 0;
> +     int i, j;
>       int err;
> +     int row, chn;
> +     bool per_rank = false;
> +
> +     BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
> +     /*
> +      * Calculate the total amount of dimms and csrows/cschannels while
> +      * in the old API emulation mode
> +      */
> +     tot_dimms = 1;
> +     tot_channels = 1;
> +     tot_csrows = 1;
> +     for (i = 0; i < n_layers; i++) {
> +             tot_dimms *= layers[i].size;
> +             if (layers[i].is_virt_csrow)
> +                     tot_csrows *= layers[i].size;
> +             else
> +                     tot_channels *= layers[i].size;
> +
> +             if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
> +                     per_rank = true;


-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists