Variables scope guideline

[jajanusz]

I'd like to drop rule that is currently enforced in FW codebase - Declaring all variables at the beginning of function. In the past it was a thing in the kernel code, but it is no more - also it makes code less readable. I think we should allow for declaring new variables at start of any block - it's like that in kernel sound's tree.

[plbossart]

Meh. that also provides a scope for shadowing variables in different scopes which will need to be trapped.
We also don't do this for Intel sound kernel code, it's not a recommended practice as you imply.

[jajanusz]

Meh. that also provides a scope for shadowing variables in different scopes which will need to be trapped.
We also don't do this for Intel sound kernel code, it's not a recommended practice as you imply.

You do f.e. here: https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/core.c#L230

[dbaluta]

NACK. I understand the need for breaking this rule, for example inside loops but in the end it can cause more trouble then gains. So, I'm against it.

[tlauda]

@dbaluta Is this our internal rule or some generic kernel one? Because if it's internal, then we need some kind of code style tutorial, so the people wouldn't try to enforce their own styles.

[plbossart]

Meh. that also provides a scope for shadowing variables in different scopes which will need to be trapped.
We also don't do this for Intel sound kernel code, it's not a recommended practice as you imply.
You do f.e. here: https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/core.c#L230

that's code from one contributor who has their own ideas about coding style, and sometimes maintainers have to pick their battles... It's not a general rule at all and not encouraged in any sound drivers to the best of my knowledge. In fact, I have never seen a review comment asking anyone to move the variable declarations in the scope where they are used.

[jajanusz]

There are many questionable guidelines in kernel, but one of our goals is to be style-compilant with kernel, so if rules are explicit, then let them be our guidelines also.
This one is unspecified and I'm asking to not be more restrictive than kernel is.

Since there are no specific guideline we can choose whatever is better by looking at pros&cons (instead of doing something because it's like this in kernel when it is not). Only disadvantage so far that was mentioned is shadowing - but you can still shadow variables at the top of function's body, so you'll end up with having them shadowed even in bigger scope.
On the other hand with smaller scopes, you have more control over variables and there is less place to misuse them. It's also easier to read such code.

I don't want to encourage anyone to move variables to smaller scope, but I want developers to be able to do so if it improves code readability/manageability.

[ranj063]

@jajanusz I think it might still be worth keeping the rule to stick with declaring variables at the beginning. FWIW, it will force developers (to a certain extent) to keep functions short and readable.

[jajanusz]

@ranj063 Yea, in ideal world you would have nice short functions where it's not an issue, but most of developers just want to have a job done and they won't split function into few smaller ones, especially when you have series of additive commits - when developer adds something to existing function. F.e. ssp_set_config that have like 30 variables at beginning and they want just add 1 feature that requires few lines, but introduces another 1 or 2 variables - you'd have to tell poor guy to refactor few hundreds lines function just to add 1 minor thing, of course dev is not going to do it, so you'll end up with him adding 31st variable to pile of variables with few hundreds line scope and with current guidelines it's more preferable solution, than having variable in smaller scope where it's needed ...

[jajanusz]

@ranj063 So to sum it up - in the world with deadlines etc it's low cost solution to something that would require a lot of work, to have it done ideally, but is still better than alternative.

[dbaluta]

@dbaluta Is this our internal rule or some generic kernel one? Because if it's internal, then we need some kind of code style tutorial, so the people wouldn't try to enforce their own styles.

@tlauda my impression from the beginning is that we are mostly following the linux kernel coding style. We (SOF project) do not have rules per se. I think it is better to follow as much as possible the Linux kernel coding style. It has been proven for 30 years that it works and generates good quality code.

I do understand that sometimes it might be hard to get used to it. I'm open to have a discussion about how much can we deviate to Linux kernel coding style but my feeling is that we do not gain to much from this at the cost of spending time otherwise useful in other parts of the project.