Re: [HACKERS] cast result of copyNode()

[Peter Eisentraut]

On 3/21/17 18:52, Mark Dilger wrote:
> 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.

Committed that.

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

And that.

The distinction between Node and Expr is more theoretical and not
handled very ridigly throughout the code.  However, your patch seemed
like a gentle improvement in relatively new code, so it seems like a
good change.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] cast result of copyNode()

[David Steele]

On 3/21/17 6:52 PM, Mark Dilger wrote:



On Mar 21, 2017, at 2:13 PM, David Steele  wrote:

Hi Mark,

On 3/9/17 3:34 PM, Peter Eisentraut wrote:

On 3/7/17 18:27, Mark Dilger wrote:

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


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?


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.


I have marked this "Waiting on Author" pending Peter's input.

--
-David
da...@pgmasters.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] cast result of copyNode()

[Mark Dilger]

> On Mar 21, 2017, at 2:13 PM, David Steele  wrote:
> 
> Hi Mark,
> 
> On 3/9/17 3:34 PM, Peter Eisentraut wrote:
>> On 3/7/17 18:27, Mark Dilger wrote:
>>> 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))
>> 
>> 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?

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



ideas_on_top_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] cast result of copyNode()

[David Steele]

Hi Mark,

On 3/9/17 3:34 PM, Peter Eisentraut wrote:

On 3/7/17 18:27, Mark Dilger wrote:

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


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
da...@pgmasters.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] cast result of copyNode()

[Peter Eisentraut]

On 3/7/17 18:27, Mark Dilger wrote:
> 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))

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

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 696e605c358e7ca5786a13c82504e9d7dbed5eb4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 9 Mar 2017 15:18:59 -0500
Subject: [PATCH v2] Cast result of copyObject() to correct type

copyObject() is declared to return void *, which allows easily assigning
the result independent of the input, but it loses all type checking.

If the compiler supports typeof or something similar, cast the result to
the input type.  This creates a greater amount of type safety.  In some
cases, where the result is assigned to a generic type such as Node * or
Expr *, new casts are now necessary, but in general casts are now
unnecessary in the normal case and indicate that something unusual is
happening.
---
 config/c-compiler.m4  | 27 +
 configure | 40 +++
 configure.in  |  1 +
 src/backend/bootstrap/bootstrap.c |  4 ++--
 src/backend/commands/copy.c   |  2 +-
 src/backend/commands/createas.c   |  2 +-
 src/backend/commands/event_trigger.c  |  8 +++
 src/backend/commands/prepare.c|  4 ++--
 src/backend/commands/view.c   |  2 +-
 src/backend/nodes/copyfuncs.c |  8 +++
 src/backend/optimizer/path/indxpath.c |  4 ++--
 src/backend/optimizer/plan/createplan.c   |  6 ++---
 src/backend/optimizer/plan/initsplan.c|  8 +++
 src/backend/optimizer/plan/planagg.c  |  4 ++--
 src/backend/optimizer/plan/planner.c  |  4 ++--
 src/backend/optimizer/plan/setrefs.c  | 26 ++--
 src/backend/optimizer/plan/subselect.c| 14 +--
 src/backend/optimizer/prep/prepjointree.c |  4 ++--
 src/backend/optimizer/prep/preptlist.c|  2 +-
 src/backend/optimizer/prep/prepunion.c|  4 ++--
 src/backend/optimizer/util/tlist.c| 12 +-
 src/backend/parser/analyze.c  |  2 +-
 src/backend/parser/gram.y |  2 +-
 src/backend/parser/parse_clause.c |  2 +-
 src/backend/parser/parse_expr.c   |  2 +-
 src/backend/parser/parse_relation.c   |  2 +-
 src/backend/parser/parse_utilcmd.c|  6 ++---
 src/backend/rewrite/rewriteHandler.c  |  8 +++
 src/backend/rewrite/rewriteManip.c|  8 +++
 src/backend/tcop/postgres.c   |  6 ++---
 src/backend/utils/cache/plancache.c   | 14 +--
 src/backend/utils/cache/relcache.c|  8 +++
 src/include/nodes/nodes.h |  8 ++-
 src/include/optimizer/tlist.h |  4 ++--
 src/include/pg_config.h.in|  6 +
 src/include/pg_config.h.win32 |  6 +
 36 files changed, 178 insertions(+), 92 deletions(-)

diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 7d901e1f1a..7afaec5f85 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -178,6 +178,33 @@ fi])# PGAC_C_STATIC_ASSERT
 
 
 
+# PGAC_C_TYPEOF
+# -
+# Check if the C compiler understands typeof or a variant.  Define
+# HAVE_TYPEOF if so, and define 'typeof' to the actual key word.
+#
+AC_DEFUN([PGAC_C_TYPEOF],
+[AC_CACHE_CHECK(for typeof, pgac_cv_c_typeof,
+[pgac_cv_c_typeof=no
+for pgac_kw in typeof __typeof__ decltype; do
+  AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],
+[int x = 0;
+$pgac_kw(x) y;
+y = x;
+return y;])],
+[pgac_cv_c_typeof=$pgac_kw])
+  test "$pgac_cv_c_typeof" != no && break
+done])
+if test "$pgac_cv_c_typeof" != no; then
+  AC_DEFINE(HAVE_TYPEOF, 1,
+[Define to 1 if your compiler understands `typeof' or something similar.])
+  if test "$pgac_cv_c_typeof" != typeof; then
+AC_DEFINE(typeof, $pgac_cv_c_typeof, [Define to how the compiler spells `typeof'.])
+  fi
+fi])# PGAC_C_TYPEOF
+
+
+
 # PGAC_C_TYPES_COMPATIBLE
 # ---
 # Check if the C compiler understands __builtin_types_compatible_p,
diff --git a/configure b/configure
index b5cdebb510..47a1a59e8e 100755
--- a/configure
+++ b/configure
@@ -11399,6 +11399,46 @@ if test x"$pgac_cv__static_assert" = xyes ; then
 $as_echo "#define HAVE__STATIC_ASSERT 1" >>confdefs.h
 
 fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for typeof" >&5
+$as_echo_n "checking for typeof... " >&6; }
+if ${pgac_cv_c_typeof+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_cv_c_typeof=no
+for pgac_kw in typeof __typeof__ decltype; do
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+int x = 0;
+$pgac_kw(x) y;
+y = x;
+return y;
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile 

Re: [HACKERS] cast result of copyNode()

[Tom Lane]

Mark Dilger  writes:
> 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:

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] cast result of copyNode()

[Mark Dilger]

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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] cast result of copyNode()

[Peter Eisentraut]

On 12/31/16 11:56 AM, Tom Lane wrote:
> But doesn't this result in a boatload of warnings on compilers that
> don't have typeof()?

> Also, if your answer is "you shouldn't get any warnings because
> copyObject is already declared to return void *", then why aren't
> we just relying on that today?

Currently, you don't get any warnings, and that would continue to be the
case if a compiler doesn't have typeof().

The casts that are currently there are (apparently) merely for style,
same as casting the result of malloc().

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.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] cast result of copyNode()

[Tom Lane]

Peter Eisentraut  writes:
> In order to reduce the number of useless casts and make the useful casts
> more interesting, here is a patch that automatically casts the result of
> copyNode() back to the input type, if the compiler supports something
> like typeof(), which most current compilers appear to.  That gets us
> some more type safety and we only need to retain the casts that actually
> do change the type.

But doesn't this result in a boatload of warnings on compilers that
don't have typeof()?

If typeof() were in C99 I might figure that is an okay tradeoff,
but it isn't.  I'm not sure that this is the most useful place
to be moving our C standards compliance expectations by 20 years
in one jump.

Also, if your answer is "you shouldn't get any warnings because
copyObject is already declared to return void *", then why aren't
we just relying on that today?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers