Thread: [HACKERS] cast result of copyNode()
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. -- 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
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
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
> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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.
Mark Dilger <hornschnorter@gmail.com> 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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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 david@pgmasters.net
> On Mar 21, 2017, at 2:13 PM, David Steele <david@pgmasters.net> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 3/21/17 6:52 PM, Mark Dilger wrote: > >> On Mar 21, 2017, at 2:13 PM, David Steele <david@pgmasters.net> 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 david@pgmasters.net
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