RE: [Proposal] Level4 Warnings show many shadow vars

From: Ranier Vilela <ranier_gyn(at)hotmail(dot)com>
To: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: [Proposal] Level4 Warnings show many shadow vars
Date: 2019-12-09 01:30:33
Message-ID: MN2PR18MB2927A03176C52BBA02D449AFE3580@MN2PR18MB2927.namprd18.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

>I don't think I'm actually on board with the goal here.
Ok, I understand.

>Basically, if we take this seriously, we're throwing away the notion of
>nested variable scopes and programming as if we had just a flat namespace.
>That wasn't any fun when we had to do it back in assembly-code days, and
>I don't see a good reason to revert to that methodology today.
In general I think the use global variables its a bad design. But I understand the use.

>In a few of these cases, like the RedoRecPtr changes, there *might* be
>an argument that there's room for confusion about whether the code could
>have meant to reference the similarly-named global variable. But it's
>just silly to make that argument in places like your substitution of
>/days/ndays/ in date.c.
I would rather fix everything, including days name.

>Based on this sample, I reject the idea that we ought to be trying to
>eliminate this class of warnings just because somebody's compiler can be
>induced to emit them. If you want to make a case-by-case argument that
>particular situations of this sort are bad programming style, let's have
>that argument by all means. But it needs to be case-by-case, not just
>dropping a large patch on us containing a bunch of unrelated changes
>and zero specific justification for any of them.
This is why I suggested activating the alert in the development and review process, so that any cases that arose would be corrected very early.

>IOW, I don't think you've taken to heart Robert's upthread advice that
>this needs to be case-by-case and based on literary not mechanical
>considerations.
Ok, right.
But I was working on the second class of shadow variables, which are local variables, within the function itself, where the patch would lead to a removal of the variable declaration, maintaining the same logic and functionality, which would lead to better performance and reduction. of memory usage as well as very small.
In that case, too, would it have to be case by case?
Wow, there are many and many shadow variables ...

>BTW, if we do go forward with changing the RedoRecPtr uses, I'm not
>in love with "XRedoRecPtr" as a replacement parameter name; it conveys
>nothing much, and the "X" prefix is already overused in that area of
>the code. Perhaps "pRedoRecPtr" would be a better choice? Or maybe
>make the local variables be all-lower-case "redorecptr", which would
>fit well in context in places like
pRedoRecPtr, It's perfect for me.

regards,
Ranier Vilela

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Craig Ringer 2019-12-09 02:22:12 Re: ssl passphrase callback
Previous Message Ranier Vilela 2019-12-09 01:16:22 RE: [Proposal] Level4 Warnings show many shadow vars