Re: [patch 04/91] proc: save LOC in __xlate_proc_name()

Re: [patch 04/91] proc: save LOC in __xlate_proc_name()

[Alexey Dobriyan]

On Thu, May 06, 2021 at 07:24:36PM -0700, Linus Torvalds wrote:
> On Thu, May 6, 2021 at 6:02 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > From: Alexey Dobriyan <adobriyan@gmail.com>
> > Subject: proc: save LOC in __xlate_proc_name()
> ..
> > +       while ((next = strchr(cp, '/'))) {
> 
> Please don't do this.

It is actually how it should be done.

Kernel has such code in other places

	#define hlist_for_each(pos, head) \
	        for (pos = (head)->first; pos ; pos = pos->next)

And we do check pointers for validness like this

	if (ptr) {
	}

"while" loop is no different.

> Yes, gcc suggests that double parentheses syntax around an assignment
> to avoid warnings.

I never saw this warning. I just wrote double parenth knowing the
warning will be emitted. It's an old warning.

> gcc is wrong, and is being completely stupid.

> The proper way to avoid the "assignment in conditional" warning is to
> (surprise, surprise) USE A CONDITIONAL.
> 
> So that
> 
>           while ((next = strchr(cp, '/'))) {
> 
> is the crazy rantings of a misguided compiler. No sane human should
> ever care about some odd double parenthesis syntax. We're not writing
> LISP, for chrissake.
> 
> The proper way to write this is
> 
>           while ((next = strchr(cp, '/')) != NULL) {

This NULL is redundant in the same way "if (ptr != NULL)" is redundant.
Even more so in C where comparison can't be overloaded.
You don't even save the parenth.

> which makes sense to not just a machine, but to a human, and avoids
> the whole "assignment used as a conditional" warning very naturally.
> 
> See? Now it uses a conditional as a conditional. Doesn't that make a
> whole lot more sense than the crazy ramblings of a broken machine
> mind?
> 
> I fixed it up manually, I just wanted to rant against this kind of
> "mindlessly take advice from the compiler without thinking about it".

Whatever-by: Alexey Dobriyan <adobriyan@gmail.com>

Re: [patch 04/91] proc: save LOC in __xlate_proc_name()

[Linus Torvalds]

On Thu, May 6, 2021 at 10:24 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> On Thu, May 06, 2021 at 07:24:36PM -0700, Linus Torvalds wrote:
> > ..
> > > +       while ((next = strchr(cp, '/'))) {
> >
> > Please don't do this.
>
> It is actually how it should be done.

No, Alexey, it really isn't.

> Kernel has such code in other places

.. and then you show that you do not understand the problem at all.

It's the unacceptable "double parentheses" disease I'm talking about.
The "other places" you bring up DO NOT DO THAT.

> "while" loop is no different.

This is not at all about the while loop. Comprende?

We don't do

   if ((a = b)) ..

or

   while ((a = b)) ..

or anything like that.

Why? Because that is pure and utter crazy garbage. The above syntax is
just insane.

> I never saw this warning. I just wrote double parenth knowing the
> warning will be emitted. It's an old warning.

Yes, and you picked the wrong solution for it. You picked the "write
bad code just to make the warning go away".

We don't make warnings go away by writing insane and bad code. We
write sane code.

So I repeat:

> > The proper way to write this is
> >
> >           while ((next = strchr(cp, '/')) != NULL) {

Notice the difference?

Because the "hide a warning by adding random parentheses" is not the
way we do things in the kernel.

So yes, we also write

    while (next) {..

and you are correct that this case doesn't need the " != NULL".

But notice how it also doesn't need the crazy extra parentheses?

           Linus