Subject: Re: cast result of copyObject()

From: Peter Eisentraut <[email-masked]>
Date: Tue, 28 Feb 2017 22:21:18 -0500

> The problem this patch would address is that currently you can write
> 
> SomeNode *x = ...;
> 
> ...
> 
> 
> OtherNode *y = copyObject(x);
> 
> and there is no notice about that potential mistake.
> 
> This patch makes that an error.
> 
> If you are sure about what you are doing, you can add a cast.  But
> casting the result of copyObject() should be limited to a few specific
> cases where the result is assigned to a generic Node * or something like
> that.

Updated patch, which I will register in the commit fest for some wider
portability testing (Windows!).

(changed subject copyNode -> copyObject (was excited about castNode at
the time))

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Mark Dilger <[email-masked]>
Date: Tue, 07 Mar 2017 23:27:12 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi Peter,

I like the patch so far, and it passes all the regression tests for me on both mac and linux. I
am very much in favor of having more compiler type checking, so +1 from me.

You appear to be using a #define macro to wrap a function of the same name
with the code:

#define copyObject(obj) ((typeof(obj)) copyObject(obj))

I don't necessarily have a problem with that, but it struck me as a bit odd, and
grep'ing through the sources, I don't see anywhere else in the project where that
is done for a function of the same number of arguments, and only one other
place where it is done at all:

$ find src/ -type f -name "*.h" -or -name "*.c" | xargs cat | perl -e 'while(<>) { print if (/^#define (\w+)\b.*\b\1\s*\(\b/); }'
#define copyObject(obj) ((typeof(obj)) copyObject(obj))
#define mkdir(a,b)	mkdir(a)

I'm just flagging that for discussion if anybody thinks it is too magical.
-- 
Sent via pgsql-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Mar 2017 18:34:15 -0500

> place where it is done at all:

I agree that that seems like a bad idea.  Better to rename the underlying
function --- for one thing, that provides an "out" if some caller doesn't
want this behavior.

			regards, tom lane

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 9 Mar 2017 15:34:30 -0500

> #define copyObject(obj) ((typeof(obj)) copyObject(obj))

Yeah, that's a bit silly.  Here is an updated version that changes that.

---

From: David Steele <[email-masked]>
Date: Tue, 21 Mar 2017 17:13:23 -0400

Hi Mark,

> Yeah, that's a bit silly.  Here is an updated version that changes that.

Do you know when you'll have a chance to take a look at the updated patch?

-- 
-David
[email-masked]

---

From: Mark Dilger <[email-masked]>
Date: Tue, 21 Mar 2017 15:52:10 -0700

> On Mar 21, 2017, at 2:13 PM, David Steele <[email-masked]> wrote:
> 
> Hi Mark,
> 

The patch applies cleanly, compiles, and passes all the regression tests
for me on my laptop.  Peter appears to have renamed the function copyObject
as copyObjectImpl, which struct me as odd when I first saw it, but I don't have
a better name in mind, so that seems ok.

If the purpose of this patch is to avoid casting so many things down to (Node *),
perhaps some additional work along the lines of the patch I'm attaching are
appropriate.  (This patch applies on top Peter's v2 patch).  The idea being to
keep objects as (Expr *) where appropriate, rather than casting through (Node *)
quite so much.

I'm not certain that this is (a) merely a bad idea, (b) a different idea than what
Peter is proposing, and as such should be submitted independently, or
(c) something that aught to be included in Peter's patch prior to commit.
I only applied this idea to one file, and maybe not completely in that file, because
I'd like feedback before going any further along these lines.

mark