Re: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Vaittinen, Matti]

On 8/21/22 16:13, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 10:00 PM Matti Vaittinen
> <mazziesaccount@xxxxxxxxx> wrote:
>> On 8/20/22 20:41, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 8:30 PM Matti Vaittinen
>>> <mazziesaccount@xxxxxxxxx> wrote:
>>>> On 8/20/22 19:09, Andy Shevchenko wrote:
>>>>> On Sat, Aug 20, 2022 at 4:45 PM Matti Vaittinen
>>>>> <mazziesaccount@xxxxxxxxx> wrote:
>>>>>> On 8/20/22 14:21, Jonathan Cameron wrote:
>>>>>>> On Fri, 19 Aug 2022 22:19:17 +0300
>>>>>>> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>>>
>>> ...
>>>
>>>>>>> For the whole static / vs non static. My personal preference is not
>>>>>>> to have the static marking but I don't care that much.
>>>>>>
>>>>>> I'd like to stick with the static here. I know this one particular array
>>>>>> does not have much of a footprint - but I'd like to encourage the habit
>>>>>> of considering the memory usage. This discussion serves as an example of
>>>>>> how unknown the impact of making const data static is. I didn't know
>>>>>> this myself until Sebastian educated me :)  Hence my strong preference
>>>>>> on keeping this 'static' as an example for others who are as ignorant as
>>>>>> I were ;) After all, having const data arrays static is quite an easy
>>>>>> way of improving things - and it really does matter when there is many
>>>>>> of arrays - or when they contain large data.
>>>>>
>>>>> But still the same comment about global scope of the variable is applied.
>>>>
>>>> I don't understand why you keep claiming the variable is global when it
>>>> is not?
>>>
>>> It is. The static keyword makes it global, but putting the entire
>>> definition into the function is asking for troubles.
> 
>> Please, describe the trouble we can get with a local static const
>> variable? A real concrete threat there is. I have explained the benefit.
>> I have also explained the concrete possibility of name collision when we
>> really do a global out of local.
> 
> I told you, the benefit is not to play dirty tricks on developers,
> maintainers and reviewers.

I see nothing concrete in that statement.

> It's simply harder to read the code and get
> the usage of the variable that lifetime is out of scope of the
> function.

This still makes no sense to me. Lifetime is the same even if we put the 
variable out of the function as you suggested. It does not change. And a 
reviewer missing that fact for a const data does really not matter. 
Variable is there when function is entered, it has always the same value 
- it just is not in the stack. I agree that a variable which value may 
change between function calls is more difficult to track when it is 
static. This is not the case here.

For a reviewer or code reader it is actually _much simpler_ to see where 
the variable is used when we put it in the block where it is used. If I 
did as you suggested and pulled it out of the function then every code 
reader should grep the whole file to detect the use. Now readers only 
need to check if a pointer to the variable is returned out of the 
function. Oh, and this same check should be done for truly global 
variables too - as users can store the pointer to something which does 
not match the grep.

So, again. Putting the variables (also static ones) in the blocks do 
make code reading and reviewing _easier_.

> 
>>> I guess some C standard chapter describes that in non-understandable language.
>>>
>>>>> As I explained before, hiding global variables inside a function is a
>>>>> bad code practice.
>>>>
>>>> I don't really get what you mean here. And I definitely don't see any
>>>> improvement if we would really use a global variable instead of a local one.
>>>
>>> The improvement is avoid hiding the global variable to the local namespace.
>>
>> I guess you mean that you may miss the fact that a variable stays there
>> even after execution exits the function, right? Ok, let's assume someone
>> misses this point when reading the code. Now, please describe me the
>> potential issues this can cause knowing our static is const and doesn't
>> change the value.
> 
> When you hide the static variable inside the function, you simply
> narrow visibility to the compiler, but the variable stays all the time
> the module is in.

Yes. The constant, unchanging data stays there all the time. How does it 
make your reviewing harder? Why do you care whether the data stays in 
the same place or not? What you should be interested is where and how 
the data is accessed - and this is where having the variable local will 
actually help you.

More I think of this, less I can see the problem you see :(

Best Regards
	-- Matti

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~