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