Re: [Proposal] Level4 Warnings show many shadow vars - Mailing list pgsql-hackers
From | Stephen Frost |
---|---|
Subject | Re: [Proposal] Level4 Warnings show many shadow vars |
Date | |
Msg-id | 20191211153438.GV6962@tamriel.snowman.net Whole thread Raw |
In response to | RE: [Proposal] Level4 Warnings show many shadow vars (Ranier Vilela <ranier_gyn@hotmail.com>) |
List | pgsql-hackers |
Greetings, Didn't see this previously (it's our typical approach to 'reply-all' to people), though I don't think it changes my feelings about the latest proposed patch. * Ranier Vilela (ranier_gyn@hotmail.com) wrote: > De: Stephen Frost > Enviadas: Terça-feira, 10 de Dezembro de 2019 17:52 > > >There's multiple ways to get there though and I think what you're seeing > >is that the "just change it to something else" answer isn't necessairly > >going to be viewed as an improvement (or, at least, not enough of an > >improvement to accept the cost of the change). > Well, I was trying to apply another non-implied rule, "break nothing". 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. > >Why not change the variables? Changes that also improve the code itself > >along with eliminating the shadowing of the global variable are going to > >be a lot easier to be accepted. > Contrary to what I was initially thinking, it seems to me that changing the names of global variables is more acceptableto the people of the project. 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. > >Sure, but have you looked at how it's used? Instead of just renaming > >the numTables variables in the functions that accept it- could those > >variables just be removed instead of changing their name to make it look > >like they're something different when they aren't actually different? > No. I didn't look. 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. > >I've only spent a bit of time looking at it, but it sure looks like the > >variables could just be removed, and doing so doesn't break the > >regression tests, which supports the idea that maybe there's a better > >way to deal with those particular variables rather than renaming them. > >Another approach to consider might be to move some global variables into > >structures that are then global with better names to indicate that's > >what they are. > It does not seem reasonable to me what you are asking. > Because as I was told here and I agree in part. I do not have the necessary knowledge of structures and logic to proposebig changes. 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. > For the work I set out to, find bugs and make minor performance improvements, I believe, can contribute safely and withoutruining anything. 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. > By just changing the names of variables to something consistent and readable, the goal will be done without break anything. 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?" > Who is best to make these changes, are the authors and reviewers. Once we no longer have the problem of shadow variables,we can turn on the alert without breaking automatic compilation, as Tom Lane is concerned. 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. 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". > That's why I'd like to fix all collisions of variables, even the dumbest. I agree with fixing collisions, but not in a rote way like this. > >In short, a hack-and-slash patch that doesn't really spend much time > >considering the changes beyond "let's just change these to be different > >to avoid shadowing globals" isn't really a good way to go about > >addressing these cases and has a good chance of making things more > >confusing, not less. > This is totally contrary to what I think about it. -1 from me then on this whole thread of changes. Thanks, Stephen
Attachment
pgsql-hackers by date: