On 8/20/22 19:21, Andy Shevchenko wrote:
On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
<mazziesaccount@xxxxxxxxx> wrote:
On 8/20/22 10:18, Andy Shevchenko wrote:
On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
<Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
On 8/20/22 09:25, Andy Shevchenko wrote:
On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
<Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
On 8/20/22 02:30, Andy Shevchenko wrote:
On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<mazziesaccount@xxxxxxxxx> wrote:
What did I miss?
>>>> struct bmg160_data *data;
>>>> struct iio_dev *indio_dev;
This does already violate the rule.
Indeed, I am reading this with an MTA that has True Type fonts, and I
can't see it at the first glance. But this breaks that rule slightly
while your added line breaks it significantly.
Yes. As I said, I think the reverse xmas tree rule is not too well
justified. Bunch of the subsystems do not really follow it, nor did this
function. Yet, as I said, I can move the array to the first line in the
function when I respin the series..
You still can do better in _your_ series, right?
I don't see the benefit of the reverse xmas tree. We have discussed this
already in the past :) I definitely have no need to start using reverse
xmas tree thingee somewhere it has not been previously used. It may be
better in _your_ opinion.
this case you even can move it out of the function, so we will see
clearly that this is (not a hidden) global variable.
Here I do disagree with you. Moving the array out of the function makes
it _much_ less obvious it is not used outside this function. Reason for
making is "static const" is to allow the data be placed in read-only
area (thanks to Guenter who originally gave me this tip).
Just wanted to correct - it was Sebastian Reichel, not Guenter who
explained me why doing local static const arrays is better than plain const.
Did he suggest putting it inside the function?
He asked me to convert a local array to static const. I though like you
do now that the local array should not be static but just const - and he
corrected me in his reply. This can be seen in the discussion I linked
below.
"static" in C language means two things (that's what come to my mind):
- for functions this tells that a function is not used outside of the module;
- for variables that it is a _global_ variable.
Hiding static inside functions is not a good coding practice since it
hides scope of the variable.
For const arrays the static in function does make sense. Being able to
place the data in read-only areas do help with the memory on limited
systems.
I'm not sure we are on the same page. I do not object to the "const"
part and we are _not_ talking about that.
Maybe the explanation by Sebastian here can put us on the same page:
https://lore.kernel.org/all/20190502073539.GB7864@localhost.localdomain/
https://lore.kernel.org/all/322fa765ddd72972aba931c706657661ca685afa.camel@xxxxxxxxxxxxxxxxx/
Again, you are too focused on "const", I'm talking about "static". The
above doesn't clear a bit regarding why you hide the global variable
inside a function. I don't see either Sebastian's clear point on this.
I don't really see why you talk about "hiding a global variable in a
function"? A static variable which is declared in function is not
global. It is local. It causes no more name collisions than a regular
local variable does so I really don't understand your reasoning.
And if you look into the kernel code, I
believe the use you are proposing is in minority.
I don't know about the statistics. What I know is that we do have a
technical benefits when we use static const arrays instead of non static
ones in the functions. I do also believe placing the variables in blocks
is a good practice.
Yes, and global variables are better to be seen as global variables.
I tend to agree with you that using local, non const statics has
pitfalls - but the pitfalls do not really apply with const ones. You
know the value and have no races. Benefit is that just by seeing that no
pointer is returned you can be sure that no "sane code" uses the data
outside the function it resides.
Putting a global variable (const or non-const) to the function will
hide its scope and it is prone to getting two variables with the same
or very similar names with quite different semantics.
I don't see how moving something from a local block to a global scope
does make conflicts less of an issue?
You may add a static variable inside each function in the same module
and name it "foo" and there will be no conflict, but when you read the
code your brain will be spoiled.
And how is it different from reading functions where the regular
variables have identical names? I _really_ can't follow your reasoning.
This is simply _bad code practice_. I
don't know how else I can explain this to you.
On the contrary, it makes things
worse as then the moved variable will collide with any other variable in
any of the functions in the whole file. Having the array as function
local static makes the naming collisions to be issue only if another
global variable has the same name.
Again, you missed my point. I'm talking about reading and analysing
the code.
I _definitely_ miss your point here. I have zero problems reading code
where static const variables are used in a function. I think it is
pretty much as hard as seeing a #defined value - difference being that
one can point to the variable.
I admit that static variables whose value is changed can be more of a
problem especially when access to function is not serialized.
Otherwise (good) compiler should spill a lot of warnings in
case you have global vs. local naming collision.
And if that happened - the chances
are code would still be correct as the function here is clearly intended
to use the local one. If someone really later adds a global with the
same name - and uses the global in this function - then he should have
noted we have local variable with same name. Additionally - such user
would be using terribly bad name for a global variable.
Please note that scope of the function local static variable is limited
to function even if the life-time is not just the life-time of a function.
Nope. The RO section might be very well flashed into ROM, so...
...so?
That's why it's
really not good practice. I would rather see it outside of the
function _esp_ because it's static const.
Sorry, I really don't agree with your reasoning here. :(
So, whom should we listen to here? Because bad code is bad code. And
this is code above.
Bad is a subjective concept. I'd say the code gets much worse if we make
the local variable a global one.
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
