Russell King - ARM Linux wrote, on 03/06/2011 01:48 PM:
On Sun, Mar 06, 2011 at 08:15:06AM +0530, Nishanth Menon wrote:my_dumb_func(){ struct voltagedomain *vdata = NULL; if (cpu_is_omap3630()_) { vdata = omap_voltage_domain_lookup("mpu") } /* forgot to add other cpu types or a else case */ /* do other things */ me_volt=omap_voltage_get_nom_volt(vdata); /* do things with me_volt */ }Sorry, but i think the check, even if seems superfluous is sane IMHO. even if the above code worked on 3630, it'd fail on o4/3430 without the check, it can even crash. and given that we've all seen our fair share of developers who develop for one platform without consideration that the rest of the world uses others as well... I do feel cases like above example might infact be a reality.But normal practice is to check the return value from functions before it's used. So: my_dumb_func(){ struct voltagedomain *vdata = NULL; if (cpu_is_omap3630()) { vdata = omap_voltage_domain_lookup("mpu") } /* forgot to add other cpu types or a else case */ + if (!vdata) + return some error; /* do other things */ me_volt=omap_voltage_get_nom_volt(vdata); /* do things with me_volt */ } is the right way to deal with this. Pushing the primary error checking down into sub-functions is stupid - where does it stop? Do you check me_volt for errors? Do you get functions which use me_volt to check for errors from that too? It's a silly idea. Put the primary error checking in the function which gets the return value. Don't bury it in sub-functions.
I agree with you on this - sub functions esp static ones should able to trust the parameters passed to it by callers. for the moment, I suggest we drop this patch from this series - it has no functional impact to the overall goal which I was attempting to achieve. Cleanups of the voltage, smartreflex layers are an ongoing activity currently and should be takenup as part of it.
-- Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
