On 2022-Oct-11, David Rowley wrote:
> I'm also keen to wait for complaints and only if we really have to,
> remove the shadow flag from being used only in the places where we
> need to.
+1
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
"The problem with the fut
On Wed, Oct 12, 2022 at 02:50:58PM +1300, David Rowley wrote:
> Thanks for finding that and coming up with the patch. It looks fine to
> me. Do you want to push it?
Thanks for double-checking. I'll do so shortly, I just got annoyed by
that for a few days :)
Thanks for your work on this thread to
On Wed, 12 Oct 2022 at 14:39, Michael Paquier wrote:
> -Wshadow=compatible-local causes one extra warning in postgres.c with
> -DWRITE_READ_PARSE_PLAN_TREES:
> postgres.c: In function ‘pg_rewrite_query’:
> postgres.c:818:37: warning: declaration of ‘query’ shadows a parameter
> [-Wshadow=compatib
On Tue, Oct 11, 2022 at 01:16:50PM +1300, David Rowley wrote:
> Aside from this issue, if anything I'd be keen to go a little further
> with this and upgrade to -Wshadow=local. The reason being is that I
> noticed that the const qualifier is not classed as "compatible" with
> the equivalently named
On Tue, 11 Oct 2022 at 06:02, Tom Lane wrote:
>
> Alvaro Herrera writes:
> > On 2022-Oct-10, Andres Freund wrote:
> >> We could, but is it really a useful thing for something fixed 6 years ago?
>
> > Well, for people purposefully installing using older installs of Perl
> > (not me, admittedly), i
Alvaro Herrera writes:
> On 2022-Oct-10, Andres Freund wrote:
>> We could, but is it really a useful thing for something fixed 6 years ago?
> Well, for people purposefully installing using older installs of Perl
> (not me, admittedly), it does seem useful, because you get the benefit
> of checkin
On 2022-Oct-10, Andres Freund wrote:
> On 2022-10-10 09:37:38 -0700, Andres Freund wrote:
> > On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote:
> > > On 2022-Oct-10, Andres Freund wrote:
> > >
> > > > Given the age of affected perl instances I suspect there'll not be a
> > > > lot of
> > > > d
On 2022-10-10 09:37:38 -0700, Andres Freund wrote:
> On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote:
> > On 2022-Oct-10, Andres Freund wrote:
> >
> > > Given the age of affected perl instances I suspect there'll not be a lot
> > > of
> > > developers affected, and the number of warnings is re
Hi,
On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote:
> On 2022-Oct-10, Andres Freund wrote:
>
> > Given the age of affected perl instances I suspect there'll not be a lot of
> > developers affected, and the number of warnings is reasonably small too.
> > It'd
> > likely hurt more developers t
On 2022-Oct-10, Andres Freund wrote:
> Given the age of affected perl instances I suspect there'll not be a lot of
> developers affected, and the number of warnings is reasonably small too. It'd
> likely hurt more developers to not see the warnings locally, given that such
> shadowing often causes
Hi,
On 2022-10-10 12:06:22 -0400, Tom Lane wrote:
> Scraping the configure logs also shows that only half of the buildfarm
> (exactly 50 out of 100 reporting animals) knows -Wshadow=compatible-local,
> which suggests that we might see more of these if they all did.
I think it's not just newness -
David Rowley writes:
> On Fri, 7 Oct 2022 at 13:24, David Rowley wrote:
>> Since I just committed the patch to fix the final warnings, I think we
>> should go ahead and commit the patch you wrote to add
>> -Wshadow=compatible-local to the standard build flags. I don't mind
>> doing this.
> Pushe
On Fri, 7 Oct 2022 at 13:24, David Rowley wrote:
> Since I just committed the patch to fix the final warnings, I think we
> should go ahead and commit the patch you wrote to add
> -Wshadow=compatible-local to the standard build flags. I don't mind
> doing this.
Pushed.
David
On Thu, 6 Oct 2022 at 13:39, Andres Freund wrote:
> I attached a patch to add -Wshadow=compatible-local to our set of warnings.
Since I just committed the patch to fix the final warnings, I think we
should go ahead and commit the patch you wrote to add
-Wshadow=compatible-local to the standard bu
On Thu, 6 Oct 2022 at 20:32, Alvaro Herrera wrote:
>
> On 2022-Oct-06, David Rowley wrote:
> > I didn't want to do it that way because all this code is in a while
> > loop and the outer "now" will be reused after it's set by the code
> > above. It's not really immediately obvious to me what reper
On 2022-Oct-06, David Rowley wrote:
> On Thu, 6 Oct 2022 at 02:34, Alvaro Herrera wrote:
> > A simpler idea might be to just remove the inner declaration, and have
> > that block set the outer var. There's no damage, since the block is
> > going to end and not access the previous value anymore.
On Thu, 6 Oct 2022 at 13:39, Andres Freund wrote:
> I attached a patch to add -Wshadow=compatible-local to our set of warnings.
Thanks for writing that and for looking at the patch.
FWIW, I'm +1 for having this part of our default compilation flags. I
don't want to have to revisit this on a yea
Hi,
On 2022-10-06 13:00:41 +1300, David Rowley wrote:
> Here's a patch which (I think) fixes the ones I missed.
Yep, does the trick for me.
I attached a patch to add -Wshadow=compatible-local to our set of warnings.
> diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> index 4713e
On Thu, 6 Oct 2022 at 11:50, David Rowley wrote:
>
> On Thu, 6 Oct 2022 at 10:40, Andres Freund wrote:
> > Your commit message said the last shadowed variable. But building with
> > -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed
> > at
> > the end). Looks like it "on
On Thu, 6 Oct 2022 at 10:40, Andres Freund wrote:
> Your commit message said the last shadowed variable. But building with
> -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
> the end). Looks like it "only" fixed it for src/, without optional
> dependencies like gssap
On Thu, 6 Oct 2022 at 02:34, Alvaro Herrera wrote:
> A simpler idea might be to just remove the inner declaration, and have
> that block set the outer var. There's no damage, since the block is
> going to end and not access the previous value anymore.
>
> diff --git a/src/bin/pgbench/pgbench.c b/
Hi,
On 2022-10-06 10:21:41 +1300, David Rowley wrote:
> Also pushed. (Thanks for saving me on that one.)
Your commit message said the last shadowed variable. But building with
-Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
the end). Looks like it "only" fixed it f
On Thu, 6 Oct 2022 at 03:19, Tom Lane wrote:
>
> David Rowley writes:
> > I've attached a draft patch for a method I was considering to fix the
> > warnings we're getting from the nested PG_TRY() statement in
> > utility.c.
>
> +1
Pushed.
> > The only warning remaining after applying the attach
David Rowley writes:
> I've attached a draft patch for a method I was considering to fix the
> warnings we're getting from the nested PG_TRY() statement in
> utility.c.
+1
> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509. I'd considered changing
On 2022-Oct-05, David Rowley wrote:
> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509. I'd considered changing this to "thenow"
> which translates to "right now" in the part of Scotland that I'm from.
> I also considered "nownow", which is used in S
On Wed, 5 Oct 2022 at 21:05, David Rowley wrote:
> 5 warnings remain. 4 of these are for PG_TRY() and co.
I've attached a draft patch for a method I was considering to fix the
warnings we're getting from the nested PG_TRY() statement in
utility.c.
The C preprocessor does not allow name overload
On Tue, 4 Oct 2022 at 15:30, Justin Pryzby wrote:
> Here, you forgot to change "val < 0".
Thanks. I made another review pass of each change to ensure I didn't
miss any others. There were no other issues, so I pushed the adjusted
patch.
5 warnings remain. 4 of these are for PG_TRY() and co.
Da
On Tue, Oct 04, 2022 at 02:27:09PM +1300, David Rowley wrote:
> On Tue, 30 Aug 2022 at 17:44, Justin Pryzby wrote:
> > Would you check if any of these changes are good enough ?
>
> I looked through v5.txt and modified it so that the fix for the shadow
> warnings are more aligned to the spreadshee
On Tue, 30 Aug 2022 at 17:44, Justin Pryzby wrote:
> Would you check if any of these changes are good enough ?
I looked through v5.txt and modified it so that the fix for the shadow
warnings are more aligned to the spreadsheet I created.
I also fixed some additional warnings which leaves just 5
On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> I really think #2s should be done last. I'm not as comfortable with
> the renaming and we might want to discuss tactics on that. We could
> either opt to rename the shadowed or shadowing variable, or both. If
> we rename the shadowing
On Thu, 25 Aug 2022 at 13:46, David Rowley wrote:
> I've attached a patch which I think improves the code in
> gistRelocateBuildBuffersOnSplit() so that there's no longer a shadowed
> variable. I also benchmarked this method in a tight loop and can
> measure no performance change from getting the
On Thu, 25 Aug 2022 at 14:08, Justin Pryzby wrote:
> Here, I've included the rest of your list.
OK, I've gone through v3-remove-var-declarations.txt, v4-reuse.txt
v4-reuse-more.txt and committed most of what you had and removed a few
that I thought should be renames instead.
I also added some ad
On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> On Wed, 24 Aug 2022 at 14:39, Justin Pryzby wrote:
> > Attached are half of the remainder of what I've written, ready for review.
>
> Thanks for the patches.
> 4. "Repurpose" (variables have the same purpose and may as well use
> th
On Wed, 24 Aug 2022 at 22:47, David Rowley wrote:
> 5. "Refactor" (fix the code to make it better)
> I have some ideas on how to fix the two #5s, so I'm going to go and do that
> now.
I've attached a patch which I think improves the code in
gistRelocateBuildBuffersOnSplit() so that there's no l
On Thu, 25 Aug 2022 at 02:00, Justin Pryzby wrote:
>
> On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> > I was hoping we'd already caught all of the #1s in 421892a19, but I
> > caught a few of those in some of your other patches. One you'd done
> > another way and some you'd done t
On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> I was hoping we'd already caught all of the #1s in 421892a19, but I
> caught a few of those in some of your other patches. One you'd done
> another way and some you'd done the rescope but just put it in the
> wrong patch. The others h
On Wed, 24 Aug 2022 at 14:39, Justin Pryzby wrote:
> Attached are half of the remainder of what I've written, ready for review.
Thanks for the patches.
I started to do some analysis of the remaining warnings and put them
in the attached spreadsheet. I put each of the remaining warnings into
a ca
On Wed, Aug 24, 2022 at 12:37:29PM +1200, David Rowley wrote:
> On Tue, 23 Aug 2022 at 14:14, Justin Pryzby wrote:
> > Actually, they didn't sneak in - what I sent are the patches which are
> > ready to
> > be reviewed, excluding the set of "this" and "tmp" and other renames which
> > you
> > di
On Tue, 23 Aug 2022 at 14:14, Justin Pryzby wrote:
> Actually, they didn't sneak in - what I sent are the patches which are ready
> to
> be reviewed, excluding the set of "this" and "tmp" and other renames which you
> disliked. In the branch (not the squished patch) the first ~15 patches were
>
On Tue, Aug 23, 2022 at 01:38:40PM +1200, David Rowley wrote:
> On Tue, 23 Aug 2022 at 13:17, Justin Pryzby wrote:
> > Attached is a squished version.
>
> I see there's some renaming ones snuck in there. e.g:
> ... in fact, there's lots of renaming, so I'll just stop looking.
Actually, they did
On Tue, 23 Aug 2022 at 13:17, Justin Pryzby wrote:
> Attached is a squished version.
I see there's some renaming ones snuck in there. e.g:
- Relation rel;
- HeapTuple tuple;
+ Relation pg_foreign_table;
+ HeapTuple foreigntuple;
This one does not seem to be in the category I mentioned:
@@ -30
On Sat, Aug 20, 2022 at 09:17:41PM +1200, David Rowley wrote:
> On Fri, 19 Aug 2022 at 16:28, Justin Pryzby wrote:
> > Let me know what I can do when it's time for round two.
>
> I pushed the modified 0001-0008 patches earlier today and also the one
> I wrote to fixup the 36 warnings about "expec
On Fri, 19 Aug 2022 at 16:28, Justin Pryzby wrote:
> Let me know what I can do when it's time for round two.
I pushed the modified 0001-0008 patches earlier today and also the one
I wrote to fixup the 36 warnings about "expected" being shadowed.
I looked through a bunch of your remaining patches
On Fri, Aug 19, 2022 at 03:37:52PM +1200, David Rowley wrote:
> I'm happy for you to take that. I'd just rather not be batting such trivial
> patches over the fence at each other for days or weeks.
Yes, thanks for that.
I read through your patch, which looks fine.
Let me know what I can do when it
On Fri, 19 Aug 2022 at 11:21, Justin Pryzby wrote:
>
> On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote:
> > Any objections to pushing this to master only?
>
> I won't object, but some of your changes are what makes backpatching this less
> reasonable (foreach_current_index and newtype
On Fri, Aug 19, 2022 at 9:21 AM Justin Pryzby wrote:
>
> On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote:
> > 0001. I'd also rather see these 4 renamed:
> ..
> > 0002. I don't really like the "my" name. I also see you've added the
> ..
> > How about just "tinfo"?
> ..
> > 0005. How ab
On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote:
> 0001. I'd also rather see these 4 renamed:
..
> 0002. I don't really like the "my" name. I also see you've added the
..
> How about just "tinfo"?
..
> 0005. How about just "tslot". I'm not a fan of "this".
..
> Since I'm not a fan of "
On Thu, Aug 18, 2022 at 5:27 PM David Rowley wrote:
>
> On Thu, 18 Aug 2022 at 17:16, Justin Pryzby wrote:
> >
> > On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:
> > > I'm probably not the only committer to want to run a mile when they
> > > see someone posting 17 or 26 patches in
On Thu, 18 Aug 2022 at 17:16, Justin Pryzby wrote:
>
> On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:
> > I'm probably not the only committer to want to run a mile when they
> > see someone posting 17 or 26 patches in an email. So maybe "bang for
> > buck" is a better method for get
On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:
> I'm probably not the only committer to want to run a mile when they
> see someone posting 17 or 26 patches in an email. So maybe "bang for
> buck" is a better method for getting the ball rolling here. As you
> know, I was recently bit
Michael Paquier writes:
> A lot of the changes proposed here update the code so as the same
> variable gets used across more code paths by removing declarations,
> but we have two variables defined because both are aimed to be used in
> a different context (see AttachPartitionEnsureIndexes() in ta
On Thu, 18 Aug 2022 at 02:54, Justin Pryzby wrote:
> The first half of the patches fix shadow variables newly-introduced in v15
> (including one of my own patches), the rest are fixing the lowest hanging
> fruit
> of the "short list" from COPT=-Wshadow=compatible-local
I wonder if it's better to
On Thu, Aug 18, 2022 at 09:39:02AM +0900, Michael Paquier wrote:
> On Thu, Aug 18, 2022 at 08:49:14AM +1000, Peter Smith wrote:
> > I'd started looking at these [1] last year and spent a day trying to
> > categorise them all in a spreadsheet (shadows a global, shadows a
> > parameter, shadows a loc
On Thu, Aug 18, 2022 at 08:49:14AM +1000, Peter Smith wrote:
> I'd started looking at these [1] last year and spent a day trying to
> categorise them all in a spreadsheet (shadows a global, shadows a
> parameter, shadows a local var etc) but I became swamped by the
> volume, and then other work/lif
On Thu, Aug 18, 2022 at 12:54 AM Justin Pryzby wrote:
>
> There's been no progress on this in the past discussions.
>
> https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com
> https://www.postgresql.org/message-id/flat/CAApHDvpqBR7u9yzW4yggjG%3DQfN%3DFZsc8Wo2ckokpQtif-%2B
There's been no progress on this in the past discussions.
https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com
https://www.postgresql.org/message-id/flat/CAApHDvpqBR7u9yzW4yggjG%3DQfN%3DFZsc8Wo2ckokpQtif-%2BiQ2A%40mail.gmail.com#2d900bfe18fce17f97ec1f00800c8e27
https://w
56 matches
Mail list logo
