Messages in this thread | | | Date | Mon, 30 Apr 2012 08:23:42 -0300 | From | Mauro Carvalho Chehab <> | Subject | Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers |
| |
Em 30-04-2012 04:59, Borislav Petkov escreveu: > 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:
With this I fully agree: you're nacking patches because it is not the way you write your code, not because the code there is doing anything wrong.
If you point anything wrong on the way I wrote, then I'll fix. Otherwise, why should I do a change that will obfuscate the code?
> 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.
The editor used by te developer is not relevant. This is not a reason to obfuscate the code.
>> +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, Mauro
| |