Re: Converting tab-complete.c's else-if chain to a switch

Hi,

On 2024-07-13 13:16:14 -0400, Tom Lane wrote:
> I wrote:
> > Per discussion elsewhere [1], I've been looking at $SUBJECT.
>
> Here's a finished patchset to perform this change.

Awesome!

> With gcc 8.5.0 I see the time drop from about 3 seconds to about 0.7.  The
> win is less noticeable with clang version 15.0 on a Mac M1: from about 0.7s
> to 0.45s.

Nice!


With gcc 15 I see:

     before   after
-O0  1.2s     0.6s
-O2  10.5s    2.6s
-O3  10.8s    2.8s

Quite a nice win.


> Of course the main point is the hope that it will work at all with MSVC, but
> I'm not in a position to test that.

It does work with just your patches applied - very nice.

The only obvious thing that doesn't is that ctrl-c without a running query
terminates psql - interrupting a running query works without terminating psql,
which is a bit weird. But that seems independent of this series.


> Subject: [PATCH v1 3/5] Install preprocessor infrastructure for
>  tab-complete.c.

Ah - I was wondering how the above would actually work when I read your intro :)


> +tab-complete.c: gen_tabcomplete.pl tab-complete.in.c
> +    $(PERL) $^ --outfile $@
> +

When I first built this with make, I got this error:
make: *** No rule to make target '/home/andres/src/postgresql/src/bin/psql/tab-complete.c', needed by 'tab-complete.o'.
Stop.
 

but that's just a a consequence of the make dependency handling... Removing
the .deps directory fixed it.



> +# The input is a C file that contains some case labels that are not
> +# constants, but look like function calls, for example:
> +#    case Matches("A", "B"):
> +# The function name can be any one of Matches, HeadMatches, TailMatches,
> +# MatchesCS, HeadMatchesCS, or TailMatchesCS.  The argument(s) must be
> +# string literals or macros that expand to string literals or NULL.

Hm, the fact that we are continuing to use the same macros in the switch makes
it a bit painful to edit the .in.c in an editor with compiler-warnings
integration - I see a lot of reported errors ("Expression is not an integer
constant expression") due to case statements not being something that the
normal C switch can handle.

Perhaps we could use a distinct macro for use in the switch, which generates
valid (but nonsensical) code, so we avoid warnings?


> +# These lines are replaced by "case N:" where N is a unique number
> +# for each such case label.  (For convenience, we use the line number
> +# of the case label, although other values would work just as well.)

Hm, using the line number seems to make it a bit harder to compare the output
of the script after making changes to the input. Not the end of the world, but
...



> +tabcomplete = custom_target('tabcomplete',
> +  input: 'tab-complete.in.c',
> +  output: 'tab-complete.c',
> +  command: [
> +    perl, files('gen_tabcomplete.pl'), files('tab-complete.in.c'),
> +    '--outfile', '@OUTPUT@',
> +    '@INPUT@',
> +  ],
> +  install: true,
> +  install_dir: dir_include_server / 'utils',
> +)

I don't think we want to install tab-complete.c?



Greetings,

Andres Freund