Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| Some nitpicking about the patch follows..
| 
| On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
| > +static long 
| > +get_vtg(const char *vdomain)
| > +{
| > +	long ret = 0;
| 
| Unnecessary initialisation.
---

Many of us work in environments where initialization is in the coding
standard. It also helps with static analysis tools. CodingStyle seems to
be silent on the point, but points to Kernighan and Ritchie, who say
"These initializations are actually unnecessary, since all are zero, but
it's a good idea to make them explicit anyway."

Any reasonable compiler will avoid doing the initialization twice...

---
| ...
| > +static int cpu_vltg_show(void *md_opt, int *value)
| > +{
| > +	int rc = 0;
| > +	if (md_opt == NULL) {
| > +		if ((*value = get_vtg("v1")) <= 0)
| > +			return -EIO;
| > +	}
| > +	else {
| > +		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
| > +		*value = opt->cpu_vltg;
| > +	}
| > +
| > +	return rc;
| > +}
| 
| int rc is unnecessary because the function always returns 0. This 
| happens in many places.
---

Wonder if he wrote it for a coding standard that requires single return
(so that the "return -EIO" would have been "rc=-EIO") and converted
it...

scott

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Heikki Orsila]

On Mon, Oct 02, 2006 at 01:58:33PM -0500, Scott E. Preece wrote:
> It also helps with static analysis tools.

I'd say those analysis tools are pretty useless if they can not handle 
trivial cases like this.

> CodingStyle seems to
> be silent on the point, but points to Kernighan and Ritchie, who say
> "These initializations are actually unnecessary, since all are zero, but
> it's a good idea to make them explicit anyway."

It was a local variable. They are not autoinitialised. Are you perhaps 
mixing this with statics and globals?

- Heikki

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

Hi!

> | Some nitpicking about the patch follows..
> | 
> | On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> | > +static long 
> | > +get_vtg(const char *vdomain)
> | > +{
> | > +	long ret = 0;
> | 
> | Unnecessary initialisation.
> 
> Many of us work in environments where initialization is in the coding
> standard. 

Well, l-k is not _this_ kind of environment.

> | > +static int cpu_vltg_show(void *md_opt, int *value)
> | > +{
> | > +	int rc = 0;
> | > +	if (md_opt == NULL) {
> | > +		if ((*value = get_vtg("v1")) <= 0)
> | > +			return -EIO;
> | > +	}
> | > +	else {
> | > +		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
> | > +		*value = opt->cpu_vltg;
> | > +	}
> | > +
> | > +	return rc;
> | > +}
> | 
> | int rc is unnecessary because the function always returns 0. This 
> | happens in many places.
> ---
> 
> Wonder if he wrote it for a coding standard that requires single return
> (so that the "return -EIO" would have been "rc=-EIO") and converted
> it...

We sometimes do that in l-k, too. But having rc=-EIO; return rc; with
single return is little extreme. Just fix it, it is easier than
debating codingstyle.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Pavel Machek<pavel@ucw.cz>
| 
| Hi!
| 
| > | > > > +static long 
| > | > > > +get_vtg(const char *vdomain)
| > | > > > +{
| > | > > > +	long ret = 0;
| > | > > 
| > | > > Unnecessary initialisation.
| > | > 
| > | > No, sorry.
| > | 
| > | In get_vtg(), if VOLTAGE_FRAMEWORK is defined then
| > | 
| > | 	ret = vtg_get_voltage(v);
| > | 
| > | is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:
| > | 
| > | 	ret = vtg_get_voltage(&vhandle);
| > | 
| > | Then "return ret;" follows. I cannot see a path where 
| > | pre-initialisation of ret does anything useful. If someone removed the
| > | #else part, the compiler would bark.
| > ---
| > 
| > True, but a good compiler should remove the dead initialization...
| 
| True, but efficient code is only one of constraints. Code should be
| easy to read, too.
| 								Pavel
| -- 
| (english) http://www.livejournal.com/~pavelmachek
| (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[Ironically, I was going to say "I think this dead horse has been
sufficiently beaten, but then I saw the picture reference in Pavel's
signature and decided that would be rude!]

I agree - the initialization is unneeded. However, it's also mostly
harmless...

scott

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

Hi!

> | > > > +static long 
> | > > > +get_vtg(const char *vdomain)
> | > > > +{
> | > > > +	long ret = 0;
> | > > 
> | > > Unnecessary initialisation.
> | > 
> | > No, sorry.
> | 
> | In get_vtg(), if VOLTAGE_FRAMEWORK is defined then
> | 
> | 	ret = vtg_get_voltage(v);
> | 
> | is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:
> | 
> | 	ret = vtg_get_voltage(&vhandle);
> | 
> | Then "return ret;" follows. I cannot see a path where 
> | pre-initialisation of ret does anything useful. If someone removed the
> | #else part, the compiler would bark.
> ---
> 
> True, but a good compiler should remove the dead initialization...

True, but efficient code is only one of constraints. Code should be
easy to read, too.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| On Mon, Oct 02, 2006 at 01:58:33PM -0500, Scott E. Preece wrote:
| > It also helps with static analysis tools.
| 
| I'd say those analysis tools are pretty useless if they can not handle 
| trivial cases like this.
| 
| > CodingStyle seems to
| > be silent on the point, but points to Kernighan and Ritchie, who say
| > "These initializations are actually unnecessary, since all are zero, but
| > it's a good idea to make them explicit anyway."
| 
| It was a local variable. They are not autoinitialised. Are you perhaps 
| mixing this with statics and globals?
---

Indeed - I answered too quickly.

However, as noted, there's no practical cost to including the
initializations and some of us do work in places where it's normallly
required.

scott
-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| On Sun, Oct 01, 2006 at 07:10:32PM +0200, Pavel Machek wrote:
| > On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
| > > Some nitpicking about the patch follows..
| > > 
| > > On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
| > > > +static long 
| > > > +get_vtg(const char *vdomain)
| > > > +{
| > > > +	long ret = 0;
| > > 
| > > Unnecessary initialisation.
| > 
| > No, sorry.
| 
| In get_vtg(), if VOLTAGE_FRAMEWORK is defined then
| 
| 	ret = vtg_get_voltage(v);
| 
| is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:
| 
| 	ret = vtg_get_voltage(&vhandle);
| 
| Then "return ret;" follows. I cannot see a path where 
| pre-initialisation of ret does anything useful. If someone removed the
| #else part, the compiler would bark.
---

True, but a good compiler should remove the dead initialization...

scott
-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

Hi!

> > > Some nitpicking about the patch follows..
> > > 
> > > On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> > > > +static long 
> > > > +get_vtg(const char *vdomain)
> > > > +{
> > > > +	long ret = 0;
> > > 
> > > Unnecessary initialisation.
> > 
> > No, sorry.
> 
> In get_vtg(), if VOLTAGE_FRAMEWORK is defined then
> 
> 	ret = vtg_get_voltage(v);
> 
> is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:
> 
> 	ret = vtg_get_voltage(&vhandle);
> 
> Then "return ret;" follows. I cannot see a path where 
> pre-initialisation of ret does anything useful. If someone removed the
> #else part, the compiler would bark.

Ahha, sorry, I did not look at this kind of context.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Heikki Orsila]

On Sun, Oct 01, 2006 at 07:10:32PM +0200, Pavel Machek wrote:
> On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
> > Some nitpicking about the patch follows..
> > 
> > On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> > > +static long 
> > > +get_vtg(const char *vdomain)
> > > +{
> > > +	long ret = 0;
> > 
> > Unnecessary initialisation.
> 
> No, sorry.

In get_vtg(), if VOLTAGE_FRAMEWORK is defined then

	ret = vtg_get_voltage(v);

is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:

	ret = vtg_get_voltage(&vhandle);

Then "return ret;" follows. I cannot see a path where 
pre-initialisation of ret does anything useful. If someone removed the
#else part, the compiler would bark.

> 
> > > +static long 
> > > +set_vtg(const char *vdomain, int val)
> > > +{
> > > +	long ret = 0;
> > 
> > here too.
> 
> Wrong again. automatic variables are not zero initialized.

My bad, this was a mistake. If VOLTAGE_FRAMEWORK is not defined, ret 
must be initialised. (the compiler would have noticed this one :-)

>> 'int i = 0;' happens in many functions.

for example, omap_pm_create_point() does this.

- Heikki

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
> Some nitpicking about the patch follows..
> 
> On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> > +static long 
> > +get_vtg(const char *vdomain)
> > +{
> > +	long ret = 0;
> 
> Unnecessary initialisation.

No, sorry.

> > +static long 
> > +set_vtg(const char *vdomain, int val)
> > +{
> > +	long ret = 0;
> 
> here too. This and 'int i = 0;' happens in many functions.

Wrong again. automatic variables are not zero initialized.

> > +	err = omap_pm_core_verify_opt(opt);
> > +	if (err != 0)
> > +		goto out;
> > +
> > +	return (void *)opt;
> > +out:
> > +	kfree(opt);
> > +	return NULL;
> > +}
> 
> Maybe use if (err != 0) { kfree(opt); return NULL; } because goto out is 
> only used once?

This is actually common kernel idiom, so it is okay to leave like
this. Your other points are ok.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html