Re: [Proposal] Level4 Warnings show many shadow vars - Mailing list pgsql-hackers

Hello.

At Mon, 9 Dec 2019 01:30:33 +0000, Ranier Vilela <ranier_gyn@hotmail.com> wrote in 
> >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.

The file-scoped variable is needed to be process-persistent in any
way. If we inhibit them, the upper-modules need to create the
persistent area instead, for example, by calling XLogInitGlobals() or
such, which makes things messier. Globality doens't necessarily mean
evil and there're reasons for -Wall doesn't warn the case. I believe
we, and especially committers are not who should be kept away from
knives for the reason that knives generally have a possibility to
injure someone.

> >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.

I might be too accustomed there, but the functions that define
overriding locals don't modify the local variables and only the
functions that don't override the globals modifies the glboals.  I see
no significant confusion here.  By the way changes like "conf_file" ->
"conffile" seems really useless as a fix patch.

> >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
wouldbe corrected very early.
 

I don't think it contributes to the argument on programming style in
any way.

> >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,
wherethe patch would lead to a removal of the variable declaration, maintaining the same logic and functionality, which
wouldlead 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 ...

As Robert said, they are harmless as far as we notice. Actual bugs
caused by variable overriding would be welcomed to fix. I don't
believe "lead to better performance and reduction (of code?)" without
an evidence since modern compilers I think are not so stupid. Even if
any, performance change in such extent doesn't support the proposal to
remove variable overrides that way.

> >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.

Anyway I strongly object to the name 'pRedoRecPtr', which suggests as
if it is a C-pointer to some variable. (And I believe we use Hungarian
notation only if we don't have a better way...)  LatestRedoRecPtr
looks better to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center