[PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Dagfinn Ilmari Mannsåker
Hi hackers,

I just noticed that psql's tab completion for ALTER TABLE … SET
TABLESPACE was treating it as any other configuration parameter and
completing with FROM DEFAULT or TO after it, instead of a list of
tablespaces.

PFA a patch that fixes this.

- ilmari
--
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


From a72d9dc1f38606a0bca8ff29b561915b8cbe9d5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <[hidden email]>
Date: Thu, 30 Aug 2018 17:36:16 +0100
Subject: [PATCH] =?UTF-8?q?Fix=20tab=20completion=20for=20ALTER=20TABLE=20?=
 =?UTF-8?q?=E2=80=A6=20SET=20TABLESPACE?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It was being treated as any other configuration parameter.
---
 src/bin/psql/tab-complete.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bb696f8ee9..f107ffb027 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1783,6 +1783,10 @@ psql_completion(const char *text, int start, int end)
  "IS_TEMPLATE", "ALLOW_CONNECTIONS",
  "CONNECTION LIMIT");
 
+ /* ALTER DATABASE <name> SET TABLESPACE */
+ else if (Matches5("ALTER", "DATABASE", MatchAny, "SET", "TABLESPACE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
+
  /* ALTER EVENT TRIGGER */
  else if (Matches3("ALTER", "EVENT", "TRIGGER"))
  COMPLETE_WITH_QUERY(Query_for_list_of_event_triggers);
--
2.18.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Dagfinn Ilmari Mannsåker
[hidden email] (Dagfinn Ilmari Mannsåker) writes:

> Hi hackers,
>
> I just noticed that psql's tab completion for ALTER TABLE … SET
> TABLESPACE was treating it as any other configuration parameter and
> completing with FROM DEFAULT or TO after it, instead of a list of
> tablespaces.

And just after hitting send, I noticed I'd typed ALTER TABLE instead of
ALTER DATABASE, including in the commit message :(

Fixed patch attached.

- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen


From e5ff67c3284dd456fe80ed8a8927e12665d68fea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <[hidden email]>
Date: Thu, 30 Aug 2018 17:36:16 +0100
Subject: [PATCH] =?UTF-8?q?Fix=20tab=20completion=20for=20ALTER=20DATABASE?=
 =?UTF-8?q?=20=E2=80=A6=20SET=20TABLESPACE?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It was being treated as any other configuration parameter.
---
 src/bin/psql/tab-complete.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bb696f8ee9..f107ffb027 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1783,6 +1783,10 @@ psql_completion(const char *text, int start, int end)
  "IS_TEMPLATE", "ALLOW_CONNECTIONS",
  "CONNECTION LIMIT");
 
+ /* ALTER DATABASE <name> SET TABLESPACE */
+ else if (Matches5("ALTER", "DATABASE", MatchAny, "SET", "TABLESPACE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
+
  /* ALTER EVENT TRIGGER */
  else if (Matches3("ALTER", "EVENT", "TRIGGER"))
  COMPLETE_WITH_QUERY(Query_for_list_of_event_triggers);
--
2.18.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

a.zakirov
Hello,

On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote:

> [hidden email] (Dagfinn Ilmari Mannsåker) writes:
> > Hi hackers,
> >
> > I just noticed that psql's tab completion for ALTER TABLE … SET
> > TABLESPACE was treating it as any other configuration parameter and
> > completing with FROM DEFAULT or TO after it, instead of a list of
> > tablespaces.
>
> And just after hitting send, I noticed I'd typed ALTER TABLE instead of
> ALTER DATABASE, including in the commit message :(
>
> Fixed patch attached.

+1. Attached patch works nicely. It applies using "patch".

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

a.zakirov
In reply to this post by Dagfinn Ilmari Mannsåker
On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote:

> [hidden email] (Dagfinn Ilmari Mannsåker) writes:
> > Hi hackers,
> >
> > I just noticed that psql's tab completion for ALTER TABLE … SET
> > TABLESPACE was treating it as any other configuration parameter and
> > completing with FROM DEFAULT or TO after it, instead of a list of
> > tablespaces.
>
> And just after hitting send, I noticed I'd typed ALTER TABLE instead of
> ALTER DATABASE, including in the commit message :(
>
> Fixed patch attached.

The patch seems reasonable. It fixes the lack of tab completion for
ALTER DATABASE ... SET TABLESPACE ... .

There is no need to patch the documentation and regression tests.

Marked as Ready for Commiter.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Tom Lane-2
Arthur Zakirov <[hidden email]> writes:
> On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> And just after hitting send, I noticed I'd typed ALTER TABLE instead of
>> ALTER DATABASE, including in the commit message :(
>> Fixed patch attached.

> The patch seems reasonable. It fixes the lack of tab completion for
> ALTER DATABASE ... SET TABLESPACE ... .

LGTM too, pushed.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Thomas Munro-3
On Fri, Sep 21, 2018 at 9:22 AM Tom Lane <[hidden email]> wrote:

> Arthur Zakirov <[hidden email]> writes:
> > On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote:
> >> And just after hitting send, I noticed I'd typed ALTER TABLE instead of
> >> ALTER DATABASE, including in the commit message :(
> >> Fixed patch attached.
>
> > The patch seems reasonable. It fixes the lack of tab completion for
> > ALTER DATABASE ... SET TABLESPACE ... .
>
> LGTM too, pushed.

+   else if (Matches5("ALTER", "DATABASE", MatchAny, "SET", "TABLESPACE"))

. o O ( hmm, we now have variadic macros )

--
Thomas Munro
http://www.enterprisedb.com

Reply | Threaded
Open this post in threaded view
|

Re: Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Tom Lane-2
Thomas Munro <[hidden email]> writes:
> . o O ( hmm, we now have variadic macros )

hmmm ... but even with variadic, C's macro facility is so weak that
I'm not sure we can reimplement these with it.  What would the
expansion look like?

(It constantly annoys me that C's so weak here.  In the language
I used for my first for-pay programming work, Bliss/11, the macro
facility could have done that easily.  That was 45 years ago.)

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Andres Freund
On 2018-09-20 18:38:36 -0400, Tom Lane wrote:
> Thomas Munro <[hidden email]> writes:
> > . o O ( hmm, we now have variadic macros )
>
> hmmm ... but even with variadic, C's macro facility is so weak that
> I'm not sure we can reimplement these with it.  What would the
> expansion look like?

There's a dirty hack to count arguments in vararg macros:
https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ

Afaict that ought to be enough to implement something like the current
macros? Whether that's too ugly or not, I don't know.


> It constantly annoys me that C's so weak here.

Yea. Weak and fragile, both.


Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2018-09-20 18:38:36 -0400, Tom Lane wrote:
>> hmmm ... but even with variadic, C's macro facility is so weak that
>> I'm not sure we can reimplement these with it.  What would the
>> expansion look like?

> There's a dirty hack to count arguments in vararg macros:
> https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ

Doesn't seem to help for this case.  What we really want is to expand
a given pattern once for each variadic argument, and I don't see how
to get there from here.

Although maybe I'm thinking too much inside-the-box.  The expansion
doesn't necessarily have to be identical to the code the macros
generate today.  In fact, that code is kinda bulky.  I wonder if
we could go over to something involving a variadic function, or
maybe an array of string-pointer constants?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Andres Freund
On 2018-09-20 19:03:16 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2018-09-20 18:38:36 -0400, Tom Lane wrote:
> >> hmmm ... but even with variadic, C's macro facility is so weak that
> >> I'm not sure we can reimplement these with it.  What would the
> >> expansion look like?
>
> > There's a dirty hack to count arguments in vararg macros:
> > https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ
>
> Doesn't seem to help for this case.  What we really want is to expand
> a given pattern once for each variadic argument, and I don't see how
> to get there from here.

Depends on whether your goal is to simplify *using* the macro, or the
infrastructure for the macro.  Afaict it should be relatively
straightforward to use a, possibly simplified, macro like referenced
above to have TailMatches(...), TailMatchesCS(...), Matches(...) that
then expand to the current *N macros.


> Although maybe I'm thinking too much inside-the-box.  The expansion
> doesn't necessarily have to be identical to the code the macros
> generate today.  In fact, that code is kinda bulky.  I wonder if
> we could go over to something involving a variadic function, or
> maybe an array of string-pointer constants?

Yea, that might be a way to simplify both the macros and the use of the
macros.  Assuming we have something like PP_NARG, ISTM it should be
relatively straightforward to define something like

#define Matches(...) CheckMatchesFor(PP_NARG(__VA_ARGS__), __VA_ARGS__)

and then have a CheckMatchesFor() first check previous_words_count, and
then just have a simple for loop through previous_words[] - afaict the
number of arguments ought to suffice to make that possible?

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Andres Freund
On 2018-09-20 16:19:26 -0700, Andres Freund wrote:

> On 2018-09-20 19:03:16 -0400, Tom Lane wrote:
> > Andres Freund <[hidden email]> writes:
> > > On 2018-09-20 18:38:36 -0400, Tom Lane wrote:
> > >> hmmm ... but even with variadic, C's macro facility is so weak that
> > >> I'm not sure we can reimplement these with it.  What would the
> > >> expansion look like?
> >
> > > There's a dirty hack to count arguments in vararg macros:
> > > https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ
> >
> > Doesn't seem to help for this case.  What we really want is to expand
> > a given pattern once for each variadic argument, and I don't see how
> > to get there from here.
>
> Depends on whether your goal is to simplify *using* the macro, or the
> infrastructure for the macro.  Afaict it should be relatively
> straightforward to use a, possibly simplified, macro like referenced
> above to have TailMatches(...), TailMatchesCS(...), Matches(...) that
> then expand to the current *N macros.
>
>
> > Although maybe I'm thinking too much inside-the-box.  The expansion
> > doesn't necessarily have to be identical to the code the macros
> > generate today.  In fact, that code is kinda bulky.  I wonder if
> > we could go over to something involving a variadic function, or
> > maybe an array of string-pointer constants?
>
> Yea, that might be a way to simplify both the macros and the use of the
> macros.  Assuming we have something like PP_NARG, ISTM it should be
> relatively straightforward to define something like
>
> #define Matches(...) CheckMatchesFor(PP_NARG(__VA_ARGS__), __VA_ARGS__)
>
> and then have a CheckMatchesFor() first check previous_words_count, and
> then just have a simple for loop through previous_words[] - afaict the
> number of arguments ought to suffice to make that possible?
Here's a very quick-and-dirty implementation of this approach. Some very
very brief testing seems to indicate it works, although I'm sure not
perfectly.

The current duplication of the new functions doing the actual checking
(like CheckMatchesFor(), CheckTailMatchesFor()) would probably need to
be reduced. But I don't want to invest actual time before we decide that
this could be something we'd actually want to pursue.

Greetings,

Andres Freund

varargify-tab-complete.diff (128K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Thomas Munro-3
On Fri, Sep 21, 2018 at 5:52 PM Andres Freund <[hidden email]> wrote:
> Here's a very quick-and-dirty implementation of this approach. Some very
> very brief testing seems to indicate it works, although I'm sure not
> perfectly.
>
> The current duplication of the new functions doing the actual checking
> (like CheckMatchesFor(), CheckTailMatchesFor()) would probably need to
> be reduced. But I don't want to invest actual time before we decide that
> this could be something we'd actually want to pursue.

+1

And here is a quick-and-dirty variadic COMPLETE_WITH(...).  Together:

    else if (Matches("PREPARE", MatchAny, "AS"))
        COMPLETE_WITH("SELECT", "UPDATE", "INSERT", "DELETE FROM");

+ * Making C pretty by making it ugly again.

Heh.

--
Thomas Munro
http://www.enterprisedb.com

0001-Variadic-Matches-.-macros.patch (172K) Download Attachment
0002-Variadic-COMPLETE_WITH-.-macros.patch (114K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Tom Lane-2
Thomas Munro <[hidden email]> writes:
> On Fri, Sep 21, 2018 at 5:52 PM Andres Freund <[hidden email]> wrote:
>> Here's a very quick-and-dirty implementation of this approach. Some very
>> very brief testing seems to indicate it works, although I'm sure not
>> perfectly.

> And here is a quick-and-dirty variadic COMPLETE_WITH(...).  Together:

I'm a bit inclined to get rid of the need for PP_NARG() by instead making
the macros add a trailing NULL argument, viz

#define TailMatches(...) \
  CheckTailMatchesFor(previous_words_count, previous_words, __VA_ARGS__, NULL)

This'd require (some of) the implementation functions to iterate over
their variadic arguments twice, the first time merely counting how many
there are.  But we aren't exactly concerned about max runtime performance
here, and the PP_NARG() thing seems like a crock that could easily blow
out compilation time on some compilers.

If people are OK with that design decision, I'm happy to assemble these
pieces and push it through.  I just had to add two more versions of
HeadMatchesN :-( so I'm feeling motivated to make something happen.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Andres Freund
Hi,

On 2018-09-21 16:20:42 -0400, Tom Lane wrote:

> Thomas Munro <[hidden email]> writes:
> > On Fri, Sep 21, 2018 at 5:52 PM Andres Freund <[hidden email]> wrote:
> >> Here's a very quick-and-dirty implementation of this approach. Some very
> >> very brief testing seems to indicate it works, although I'm sure not
> >> perfectly.
>
> > And here is a quick-and-dirty variadic COMPLETE_WITH(...).  Together:
>
> I'm a bit inclined to get rid of the need for PP_NARG() by instead making
> the macros add a trailing NULL argument, viz
>
> #define TailMatches(...) \
>   CheckTailMatchesFor(previous_words_count, previous_words, __VA_ARGS__, NULL)

I don't think that'd *quite* work right now - MatchAny is also NULL. We
probably could make it work by redefining either MatchAny or the last
argument to CheckTailMatchesFor() to some other value however.


> This'd require (some of) the implementation functions to iterate over
> their variadic arguments twice, the first time merely counting how many
> there are.

Yea, leaving the above problem aside, I've a hard time to get excited
about that overhead.


> But we aren't exactly concerned about max runtime performance
> here, and the PP_NARG() thing seems like a crock that could easily blow
> out compilation time on some compilers.

It's not actually that complicated an expansion, and we've not
encountered many problems with expansions that are similarly complex,
e.g. ereport et al.  It's pretty likely at least as cheap as the current
approach, where we sometimes have 9 deep recursion. So I'm not too
concerned about the compile-time performance. FWIW, I tested it with gcc
-E yesterday, and I couldn't measure any difference.

I think there's some argument to be made about the "mental" complexity
of the macros - if we went for them, we'd certainly need to add some
docs about how they work.  One argument for having PP_NARGS (renamed) is
that it doesn't seem useful just here, but in a few other cases as well.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Thomas Munro-3
On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <[hidden email]> wrote:
> I think there's some argument to be made about the "mental" complexity
> of the macros - if we went for them, we'd certainly need to add some
> docs about how they work.  One argument for having PP_NARGS (renamed) is
> that it doesn't seem useful just here, but in a few other cases as well.

It's a nice general facility to have in the tree.  It seems to compile
OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a
lazy way to see if AppVeyor would build it OK, and it worked fine
until conflicting commits landed).  I wonder if xlc, icc, aCC and Sun
Studio can grok it.

--
Thomas Munro
http://www.enterprisedb.com

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Andres Freund
Hi,

On 2018-09-22 09:15:27 +1200, Thomas Munro wrote:

> On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <[hidden email]> wrote:
> > I think there's some argument to be made about the "mental" complexity
> > of the macros - if we went for them, we'd certainly need to add some
> > docs about how they work.  One argument for having PP_NARGS (renamed) is
> > that it doesn't seem useful just here, but in a few other cases as well.
>
> It's a nice general facility to have in the tree.  It seems to compile
> OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a
> lazy way to see if AppVeyor would build it OK, and it worked fine
> until conflicting commits landed).  I wonder if xlc, icc, aCC and Sun
> Studio can grok it.

I think unless $compiler doesn't correctly implement vararg macros, it
really should just work.  There's nothing but pretty smart use of
actually pretty plain vararg macros.  If any of the other compilers have
troubles with that, they'd really not implement vararg macros...

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Thomas Munro-3
On Sat, Sep 22, 2018 at 9:46 AM Andres Freund <[hidden email]> wrote:

> On 2018-09-22 09:15:27 +1200, Thomas Munro wrote:
> > On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <[hidden email]> wrote:
> > > I think there's some argument to be made about the "mental" complexity
> > > of the macros - if we went for them, we'd certainly need to add some
> > > docs about how they work.  One argument for having PP_NARGS (renamed) is
> > > that it doesn't seem useful just here, but in a few other cases as well.
> >
> > It's a nice general facility to have in the tree.  It seems to compile
> > OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a
> > lazy way to see if AppVeyor would build it OK, and it worked fine
> > until conflicting commits landed).  I wonder if xlc, icc, aCC and Sun
> > Studio can grok it.
>
> I think unless $compiler doesn't correctly implement vararg macros, it
> really should just work.  There's nothing but pretty smart use of
> actually pretty plain vararg macros.  If any of the other compilers have
> troubles with that, they'd really not implement vararg macros...

I vote for doing it this way then.  It may turn out to be useful for
efficient SearchSysCache(...), DirectFunctionCall(...) and other
things like that.

--
Thomas Munro
http://www.enterprisedb.com

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> writes:
> On 2018-09-22 09:15:27 +1200, Thomas Munro wrote:
>> On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <[hidden email]> wrote:
>>> I think there's some argument to be made about the "mental" complexity
>>> of the macros - if we went for them, we'd certainly need to add some
>>> docs about how they work.  One argument for having PP_NARGS (renamed) is
>>> that it doesn't seem useful just here, but in a few other cases as well.

If you want to rename it, then to what?  VA_ARGS_NARGS, perhaps?

>> It's a nice general facility to have in the tree.

Yeah, that's a fair point.

>> It seems to compile
>> OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a
>> lazy way to see if AppVeyor would build it OK, and it worked fine
>> until conflicting commits landed).  I wonder if xlc, icc, aCC and Sun
>> Studio can grok it.

> I think unless $compiler doesn't correctly implement vararg macros, it
> really should just work.

Well, we'd find out pretty quickly if we try to use it here.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Andres Freund
On 2018-09-21 18:00:35 -0400, Tom Lane wrote:
> Andres Freund <[hidden email]> writes:
> > On 2018-09-22 09:15:27 +1200, Thomas Munro wrote:
> >> On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <[hidden email]> wrote:
> >>> I think there's some argument to be made about the "mental" complexity
> >>> of the macros - if we went for them, we'd certainly need to add some
> >>> docs about how they work.  One argument for having PP_NARGS (renamed) is
> >>> that it doesn't seem useful just here, but in a few other cases as well.
>
> If you want to rename it, then to what?  VA_ARGS_NARGS, perhaps?

I like your suggestion. I mainly didn't like the PP_ prefix.


> >> It's a nice general facility to have in the tree.
>
> Yeah, that's a fair point.
>
> >> It seems to compile
> >> OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a
> >> lazy way to see if AppVeyor would build it OK, and it worked fine
> >> until conflicting commits landed).  I wonder if xlc, icc, aCC and Sun
> >> Studio can grok it.
>
> > I think unless $compiler doesn't correctly implement vararg macros, it
> > really should just work.
>
> Well, we'd find out pretty quickly if we try to use it here.

You earlier were talking about tackling this - do you still want to? I
can otherwise, but it'll not be today, but likely tomorrow.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Tab completion for ALTER DATABASE … SET TABLESPACE

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2018-09-21 18:00:35 -0400, Tom Lane wrote:
>> If you want to rename it, then to what?  VA_ARGS_NARGS, perhaps?

> I like your suggestion. I mainly didn't like the PP_ prefix.

Sold.  The original author overcomplicated it anyway; I now have

/*
 * VA_ARGS_NARGS
 *        Returns the number of macro arguments it is passed.
 *
 * An empty argument still counts as an argument, so effectively, this is
 * "one more than the number of commas in the argument list".
 *
 * This works for up to 63 arguments.  Internally, VA_ARGS_NARGS_() is passed
 * 64+N arguments, and the C99 standard only requires macros to allow up to
 * 127 arguments, so we can't portably go higher.  The implementation is
 * pretty trivial: VA_ARGS_NARGS_() returns its 64th argument, and we set up
 * the call so that that is the appropriate one of the list of constants.
 */
#define VA_ARGS_NARGS(...) \
    VA_ARGS_NARGS_(__VA_ARGS__, \
                   63,62,61,60,                   \
                   59,58,57,56,55,54,53,52,51,50, \
                   49,48,47,46,45,44,43,42,41,40, \
                   39,38,37,36,35,34,33,32,31,30, \
                   29,28,27,26,25,24,23,22,21,20, \
                   19,18,17,16,15,14,13,12,11,10, \
                   9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
#define VA_ARGS_NARGS_( \
    _01,_02,_03,_04,_05,_06,_07,_08,_09,_10, \
    _11,_12,_13,_14,_15,_16,_17,_18,_19,_20, \
    _21,_22,_23,_24,_25,_26,_27,_28,_29,_30, \
    _31,_32,_33,_34,_35,_36,_37,_38,_39,_40, \
    _41,_42,_43,_44,_45,_46,_47,_48,_49,_50, \
    _51,_52,_53,_54,_55,_56,_57,_58,_59,_60, \
    _61,_62,_63,  N, ...) \
    (N)

from which it's pretty obvious that this really is a simple macro
call.  I'd first thought it involved macro recursion, but it doesn't.

> You earlier were talking about tackling this - do you still want to? I
> can otherwise, but it'll not be today, but likely tomorrow.

On it now.

                        regards, tom lane

12