RE: [Proposal] Level4 Warnings show many shadow vars
| От | Ranier Vilela |
|---|---|
| Тема | RE: [Proposal] Level4 Warnings show many shadow vars |
| Дата | |
| Msg-id | MN2PR18MB2927EC71FEC2709D3F762AA9E35A0@MN2PR18MB2927.namprd18.prod.outlook.com обсуждение исходный текст |
| Ответ на | [Proposal] Level4 Warnings show many shadow vars (Ranier Vilela <ranier_gyn@hotmail.com>) |
| Ответы |
Re: [Proposal] Level4 Warnings show many shadow vars
(John W Higgins <wishdev@gmail.com>)
Re: [Proposal] Level4 Warnings show many shadow vars (Stephen Frost <sfrost@snowman.net>) |
| Список | pgsql-hackers |
De: Stephen Frost
Enviadas: Quarta-feira, 11 de Dezembro de 2019 15:34
>I agree with not breaking things but that doesn't mean the only
>reasonable approach is to do the absolute minimum- you might not be
>breaking something today, but it's going to confuse people later on down
>the road and may lead to bugs being introduced due to that confusion, or
>at the very least will add to people's time to figure out what's really
>going on.
I don't know how such fixes could lead to more bugs.
https://www.postgresql.org/message-id/CA%2BTgmoZsM04VCKn4n8dsXxg_s8drPUHUafshG%3DP0edVjUS3Gew%40mail.gmail.com
and
https://www.postgresql.org/message-id/20191209090329.GC72921%40paquier.xyz
>I wasn't suggesting to change the names of the global variables in this
>specific case, though I could see that being a better approach in some
>instances- but it really depends. Each case needs to be reviewed and
>considered and the best approach taken.
Again, depending on the case, whether the best approach is to promote structure creation, variable removal, and logic changes, for now, is really beyond my reach.
>I think we need to be looking at the changes and considering them, and
>the person proposing the changes should be doing that and not just
>expecting everyone else to do so.
Again, I am considering only the range of my changes, which are minimal, so less likely to do something wrong, or hinder future development.
>I'd suggest that we work through that then and get you up to speed on
>the structures and logic- the pg_dump code is pretty ugly but the
>specific usage of numTables isn't too bad. Each of these should be
>looked at independently and thought about "what's the right way to fix
>this?" The right way isn't necessairly to just rename the variables, as
>I was saying, and doing so may lead to more confusion, not less.
This way it will take a long time to eliminate all name collisions.
>Having shadowed globals, while kinda ugly, doesn't necessairly mean it's
>a bug. I'm not sure what "minor performance improvements" are being
>claimed here but there's a whole lot of work involved in demonstrating
>that a change is a performance improvement.
I was referring to contributions like this:
https://github.com/postgres/postgres/commit/91da65f4ac2837e0792071e42b2e2101059f1b1b
and not specifically, performance improvements in this global unshadow patch.
>but.. the changes you're proposing are making them inconsistent and
>confusing when there isn't actually a difference between the global and
>the local, it's just the somewhere along the way someone thought they
>needed to pass in numTables when they really didn't, and we should go
>fix *that*, not rename the variable to something else to make someone
>later on go "wait, why did we need to pass in this variable? how is
>this different from the global?"
I'm confused here, but if you suggest removing the numTables variable is out of my reach.
Keeping the same name as the variable, the collision will continue, and the purpose of enabling -Wshadow will never be done someday.
>Perhaps I'm a bit confused, but it seems that you're the author of these
>specific changes, and I'm trying to provide feedback as to how you can
>improve what you're proposing in a way that will improve the code base
>overall and reduce the confusion while also eliminating the shadow
>variables. If the author of a patch isn't open to this kind of review
>and willing to adjust the patch to improve it because they aren't sure
>that their changes will be correct then they could at least post them
>back here and ask, or better, go look at the code and get a better
>understanding of what's going on to build confidence in the change.
I am the author of the patch.
I'm repeating myself, but come on, I don't have confidence in proposing logic-altering changes for now.
>The goal here also shouldn't be "we just want to turn on this alert, so
>we're going to make changes to the source without thinking just to
>appease the compiler".
Of course there is a difference in thinking here.
The changes I propose, in my opinion, are consistent, readable and break nothing.
>I agree with fixing collisions, but not in a rote way like this.
Glad you think alike about fixing collisions.
regards,
Ranier Vilela
>I agree with not breaking things but that doesn't mean the only
>reasonable approach is to do the absolute minimum- you might not be
>breaking something today, but it's going to confuse people later on down
>the road and may lead to bugs being introduced due to that confusion, or
>at the very least will add to people's time to figure out what's really
>going on.
I don't know how such fixes could lead to more bugs.
Currently there is a risk of having bugs by mixing access to shadow variables with macros.
I believe, that others has the same opinion.https://www.postgresql.org/message-id/CA%2BTgmoZsM04VCKn4n8dsXxg_s8drPUHUafshG%3DP0edVjUS3Gew%40mail.gmail.com
and
https://www.postgresql.org/message-id/20191209090329.GC72921%40paquier.xyz
>I wasn't suggesting to change the names of the global variables in this
>specific case, though I could see that being a better approach in some
>instances- but it really depends. Each case needs to be reviewed and
>considered and the best approach taken.
Again, depending on the case, whether the best approach is to promote structure creation, variable removal, and logic changes, for now, is really beyond my reach.
>I think we need to be looking at the changes and considering them, and
>the person proposing the changes should be doing that and not just
>expecting everyone else to do so.
Again, I am considering only the range of my changes, which are minimal, so less likely to do something wrong, or hinder future development.
>I'd suggest that we work through that then and get you up to speed on
>the structures and logic- the pg_dump code is pretty ugly but the
>specific usage of numTables isn't too bad. Each of these should be
>looked at independently and thought about "what's the right way to fix
>this?" The right way isn't necessairly to just rename the variables, as
>I was saying, and doing so may lead to more confusion, not less.
This way it will take a long time to eliminate all name collisions.
And worse, in my opinion, will continue to be adding new cases, since there is no rule, so check if this happens in the current development.
Not only are they global, there are dozens, perhaps hundreds of shadow local variables.
I was working on this second class of variables, which, in my opinion, would lead to less code, less bugs, and more security for the code, but I realize that my effort may not be worth it.>Having shadowed globals, while kinda ugly, doesn't necessairly mean it's
>a bug. I'm not sure what "minor performance improvements" are being
>claimed here but there's a whole lot of work involved in demonstrating
>that a change is a performance improvement.
I was referring to contributions like this:
https://github.com/postgres/postgres/commit/91da65f4ac2837e0792071e42b2e2101059f1b1b
and not specifically, performance improvements in this global unshadow patch.
>but.. the changes you're proposing are making them inconsistent and
>confusing when there isn't actually a difference between the global and
>the local, it's just the somewhere along the way someone thought they
>needed to pass in numTables when they really didn't, and we should go
>fix *that*, not rename the variable to something else to make someone
>later on go "wait, why did we need to pass in this variable? how is
>this different from the global?"
I'm confused here, but if you suggest removing the numTables variable is out of my reach.
Keeping the same name as the variable, the collision will continue, and the purpose of enabling -Wshadow will never be done someday.
>Perhaps I'm a bit confused, but it seems that you're the author of these
>specific changes, and I'm trying to provide feedback as to how you can
>improve what you're proposing in a way that will improve the code base
>overall and reduce the confusion while also eliminating the shadow
>variables. If the author of a patch isn't open to this kind of review
>and willing to adjust the patch to improve it because they aren't sure
>that their changes will be correct then they could at least post them
>back here and ask, or better, go look at the code and get a better
>understanding of what's going on to build confidence in the change.
I am the author of the patch.
I'm repeating myself, but come on, I don't have confidence in proposing logic-altering changes for now.
>The goal here also shouldn't be "we just want to turn on this alert, so
>we're going to make changes to the source without thinking just to
>appease the compiler".
Of course there is a difference in thinking here.
The changes I propose, in my opinion, are consistent, readable and break nothing.
>I agree with fixing collisions, but not in a rote way like this.
Glad you think alike about fixing collisions.
regards,
Ranier Vilela
В списке pgsql-hackers по дате отправления: