Re: [RFC] New parser framework for testing
On Wed, Nov 17, 2021 at 10:40:56PM +, Thomas Adam wrote: > On Wed, Nov 17, 2021 at 08:40:09PM +0100, Dominik Vogt wrote: > > 'k, the patched code didn't immediately crash, so here it is (two > > patches). Please test. > > I've applied those two patches on a branch called `new-parser`. > > So far, I've tested this on approximately five different configuration files > and haven't noticed anything unusual or anything which breaks. Which is a > good sign. I haven't found anything yet either. Anyway, we need infrastructure for automated testing. That shouldn't involve much more than a testing directory, a Makefile with a "test" target, and a couple of files that can be fed into "Read" via FvwmCommand. Could you try to assemble a list of parsing test cases from past bug reports? We don't need hundreds but a selection of relevant corner cases. > > The new code: > >... > This is sensible, and from a quick glance at what's there at the moment, it > makes sense. I also need to remember what this is all good for. Fortunately the hooks and structures are documented (cmdparser_hooks.h, cmdparser.h), and the whole code is only several hundred lines of not so bad code. The vital parts in functions.c are DeferExecution(), __execute_command_line() and execute_complex_function() which are all just 300 lines of code each. > I'm hoping that we can also derive the execute_context_t from > the parsed command as well, The execution context is something different. It is basically meant to transport the information who or what triggered an action to pieces of code that need it. For example, one might need to know if a window button was acticated by a mouse button press, or a release, from the keyboard (from a menu entry etc.), from a module, and so forth. This change was extremely successful. Before we had the execution context, there were loads of transient bugs because code had to guess how it had been called. These problems are all gone now. > I did wonder whether we might want to consider yacc/bison for the grammar of > the commands, but I've never personally been a fan of it, but it could help > wih some of the raw parsing... The biggest problem I see is that the parsed information needs to be passed to the commands. I really don't want to generate C struct types programatically, and definitely not with odd tools like lex/yacc/bison. We used them for a while, and quality of the generated code was somewhere below zero. Here's an ad-hoc list of things needed for BNF* based commands: (* or whatever syntax description we want to use.) * A precise description of the commands' syntax in BNF. * A way to express alternative syntax variants. We have several commands that have different modes of operation; for example "Move" takes certain arguments in interactive mode, some arguments indicate noninteractive mode, and some arguments are shared. * Some way to store it in the function table (at compile time without making functable.h depend on all other .h files.) * An indication in the function table whether a command uses old style parsing or BNF based. * A way to translate the BNF to structured data, either at build time (a la bison) or at run time (like the X event unions?). * An enhanced parser that triggers BNF parsing. Alternative: functions call the parser themselves and the syntax description can be kept in the local file. * The parsing code itself. * Memory management for parsed strings etc. * Rewritten functions (starting with simple ones first). * Later: An idea how to tackle meta command mosters like Style, MenuStyle or MoveResizeMaximize. > > * The "repeat" command may cause crashes or leaks. > > Weren't we thinking of removing the repeat command at one point? Yes. There was a patch on the onld branch to do that. I reverted it twelve minues after making that commit for an unknown reason. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [RFC] New parser framework for testing
On Wed, Nov 17, 2021 at 08:40:09PM +0100, Dominik Vogt wrote: > 'k, the patched code didn't immediately crash, so here it is (two > patches). Please test. I've applied those two patches on a branch called `new-parser`. So far, I've tested this on approximately five different configuration files and haven't noticed anything unusual or anything which breaks. Which is a good sign. > The new code: > > * Rewrites functions.c to use hooks that are provided by a >pluggable parser module (even at run time). > * The module that replicates the old parser behaviour is in >cmdparser_old.[ch]. > * The long term goal is to replace the _old parser with a new one >that evauates the BNF definitions and does the parsing of >function arguments before actually calling them. > * To allow that, a "parsing context" structure is passed to the >CMD_... functions. This is already implemented but not used. > * How the "parsing context" structure should look like is yet to >be defined. > * It shoul be entirely possible to convert command functions one >by one to the new type of parser. So that word does not need >to be done in a single big step. This is sensible, and from a quick glance at what's there at the moment, it makes sense. I'm hoping that we can also derive the execute_context_t from the parsed command as well, which would possibly help in unifying some of the command syntax in the future. I did wonder whether we might want to consider yacc/bison for the grammar of the commands, but I've never personally been a fan of it, but it could help wih some of the raw parsing... > Possible pitfalls: > * Bad behaviour of the fvwm_debug function because of wrong >parameters being passed. I did a quick santity check of the ones which were pulled in from your changes, and they look fine. All other parser debug is going to stderr anyway. > * The "repeat" command may cause crashes or leaks. Weren't we thinking of removing the repeat command at one point? Kindly, Thomas
Re: [PATCH] (6) Man page changes -- second attempt
On Wed, Nov 17, 2021 at 08:18:07PM +0100, Dominik Vogt wrote:
> On Wed, Nov 17, 2021 at 04:40:55PM +, Thomas Adam wrote:
> > I've also added an OVERVIEW
> > section to fvwm3all.adoc explaining how the man page is split up into
> > different sections.
>
> Shoudn't that go to fvwm3.1 as well?
I wouldn't have thought so -- in that, if you're reading fvwm3.1, you've
chosen that deliberately, or already read fvwm3all.1. In the same way that we
wouldn't add it to fvwm3{commands,menus,styles}.1
Kindly,
Thomas
[RFC] New parser framework for testing
'k, the patched code didn't immediately crash, so here it is (two patches). Please test. What this is about: > The whole point in the completed part of the work was to make the > command line parser pluggable. As far as I remember, that part > was done, but needed more testing by users. Now that the rebase > potentially contains some mistakes and may miss some things that > have changed in between, it's even more important. > > There is a slight incompatibility of the new code with some thiung > about quoting plus variable expansion. I had started a thread on > the mailing list and asked if that would annoy anybody and the > people who replied said no. Anyway (a) I cannot remember what it > was exactly (we can dig it up from the mail archive) and (b) there > won't be any progress if we make a new parser compatible with the > crappy bugs of the old one. The new code: * Rewrites functions.c to use hooks that are provided by a pluggable parser module (even at run time). * The module that replicates the old parser behaviour is in cmdparser_old.[ch]. * The long term goal is to replace the _old parser with a new one that evauates the BNF definitions and does the parsing of function arguments before actually calling them. * To allow that, a "parsing context" structure is passed to the CMD_... functions. This is already implemented but not used. * How the "parsing context" structure should look like is yet to be defined. * It shoul be entirely possible to convert command functions one by one to the new type of parser. So that word does not need to be done in a single big step. Possible pitfalls: * Watch out for changed whitespace behaviour. * A complex function name cannot start with a builtin name followed by an embedded space. * Possible to-dos in the code (look for the string '!!!'). * Bad behaviour of the fvwm_debug function because of wrong parameters being passed. * The "repeat" command may cause crashes or leaks. * The general code in functions.c may as well. * Same in CMD_... functions and various parts of fvwm that call execute_function to do their work. * Debug code needt to be removed later. Ciao Dominik ^_^ ^_^ -- Dominik Vogt From eddb78f108c0284fd7c670dd4d08811de59d2820 Mon Sep 17 00:00:00 2001 From: Dominik Vogt Date: Wed, 17 Nov 2021 20:03:57 +0100 Subject: [PATCH 1/2] Rewrite parser framework. --- fvwm/Makefile.am | 4 +- fvwm/add_window.c| 9 +- fvwm/builtins.c | 4 +- fvwm/cmdparser.h | 65 fvwm/cmdparser_hooks.h | 74 fvwm/cmdparser_old.c | 424 + fvwm/cmdparser_old.h | 11 + fvwm/colorset.c | 2 +- fvwm/conditional.c | 21 +- fvwm/conditional.h | 42 +++ fvwm/events.c| 40 +- fvwm/ewmh.c | 7 +- fvwm/ewmh_events.c | 33 +- fvwm/expand.c| 48 ++- fvwm/expand.h| 2 +- fvwm/functable.c | 66 +++- fvwm/functable.h | 25 +- fvwm/functable_complex.c | 157 fvwm/functable_complex.h | 55 +++ fvwm/functions.c | 771 ++- fvwm/functions.h | 41 +-- fvwm/fvwm.h | 49 +-- fvwm/fvwm3.c | 12 +- fvwm/menucmd.c | 3 +- fvwm/menus.c | 4 +- fvwm/misc.c | 1 + fvwm/module_interface.c | 3 +- fvwm/move_resize.c | 29 +- fvwm/placement.c | 5 +- fvwm/read.c | 14 +- fvwm/repeat.c| 6 +- fvwm/schedule.c | 3 +- fvwm/update.c| 7 +- fvwm/virtual.c | 24 +- fvwm/windowlist.c| 3 +- 35 files changed, 1373 insertions(+), 691 deletions(-) create mode 100644 fvwm/cmdparser.h create mode 100644 fvwm/cmdparser_hooks.h create mode 100644 fvwm/cmdparser_old.c create mode 100644 fvwm/cmdparser_old.h create mode 100644 fvwm/functable_complex.c create mode 100644 fvwm/functable_complex.h diff --git a/fvwm/Makefile.am b/fvwm/Makefile.am index a4bb6a0b..9ae456f9 100644 --- a/fvwm/Makefile.am +++ b/fvwm/Makefile.am @@ -19,6 +19,7 @@ fvwm3_SOURCES = \ placement.h read.h repeat.h execcontext.h schedule.h screen.h \ session.h stack.h style.h update.h virtual.h window_flags.h frame.h \ infostore.h \ + cmdparser.h cmdparser_hooks.h cmdparser_old.h functable_complex.h \ \ menus.c style.c borders.c events.c move_resize.c builtins.c \ add_window.c icons.c fvwm3.c frame.c placement.c virtual.c \ @@ -28,7 +29,8 @@ fvwm3_SOURCES = \ menubindings.c decorations.c ewmh_icons.c update.c bindings.c misc.c \ cursor.c colormaps.c modconf.c ewmh_conf.c read.c schedule.c \ menucmd.c ewmh_names.c icccm2.c windowshade.c focus_policy.c repeat.c \ - execcontext.c menugeometry.c menudim.c condrc.c infostore.c + execcontext.c menugeometry.c menudim.c condrc.c infostore.c \ + cmdparser_old.c functable_complex.c fvwm3_DEPENDENCIES = $(top_builddir)/libs/libf
Re: [PATCH] (6) Man page changes -- second attempt
On Wed, Nov 17, 2021 at 04:40:55PM +, Thomas Adam wrote: > On Wed, Nov 17, 2021 at 02:35:32PM +0100, Dominik Vogt wrote: > > On Tue, Nov 16, 2021 at 01:36:53AM +0100, Dominik Vogt wrote: > > > This is the full set of patches for splitting the man page, to be > > > applied to master. > > > > Second attempt. The style docs are not moved aound in the man > > page. The .section files have been renamed to .ad because > > asciidoc only evaluates "ifdef" in included files if they have a > > known extesion. ".adoc" cannot be used because the pattern rules > > in the Makefile.am would conflict. > > > > The patch stack has been reshuffled and patches have heen merged, > > and two new ones on top have been added. > > Thanks, Dominik. This applies cleanly now. Nice. > I've also added an OVERVIEW > section to fvwm3all.adoc explaining how the man page is split up into > different sections. Shoudn't that go to fvwm3.1 as well? Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: parser branch
On Wed, Nov 17, 2021 at 06:20:05PM +, Thomas Adam wrote: > On Wed, Nov 17, 2021 at 01:47:05PM +, Thomas Adam wrote: > > On Wed, Nov 17, 2021 at 02:38:19PM +0100, Dominik Vogt wrote: > > > I'd like to finish the parser work started in 2014. Is the old > > > branch still available somewhere? > > > > Remind me what work that was... > > I remember now. It came in two parts. The first part, you and I worked on > documenting the commands in BNF notation (back in 2016). That work is what > got carried across to fvwm. > > Then you created some experimental code to start to clean things up. > > That work was done in the mvwm repository [1], and the two branches you'll > want to look at are: Thanks, found them and rebased the patches to master. Couple of hours to fix conflicts. If you don't read from me for a while, that will be because fvwm crashes with the changed parser. ;-) > I can't really remember where we got to with that work either -- I'm hoping > this trip down memory lane might jog your memory more than it has mine. ;) The whole point in the completed part of the work was to make the command line parser pluggable. As far as I remember, that part was done, but needed more testing by users. Now that the rebase potentially contains some mistakes and may miss some things that have changed in between, it's even more important. There is a slight incompatibility of the new code with some thiung about quoting plus variable expansion. I had started a thread on the mailing list and asked if that would annoy anybody and the people who replied said no. Anyway (a) I cannot remember what it was exactly (we can dig it up from the mail archive) and (b) there won't be any progress if we make a new parser compatible with the crappy bugs of the old one. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: parser branch
On Wed, Nov 17, 2021 at 01:47:05PM +, Thomas Adam wrote: > On Wed, Nov 17, 2021 at 02:38:19PM +0100, Dominik Vogt wrote: > > I'd like to finish the parser work started in 2014. Is the old > > branch still available somewhere? > > Remind me what work that was... I remember now. It came in two parts. The first part, you and I worked on documenting the commands in BNF notation (back in 2016). That work is what got carried across to fvwm. Then you created some experimental code to start to clean things up. That work was done in the mvwm repository [1], and the two branches you'll want to look at are: * dv/new-parser https://github.com/ThomasAdam/mvwm/commits/dv/new-parser * dv/new-parser-2 https://github.com/ThomasAdam/mvwm/commits/dv/new-parser-2 You could add mvwm as a git-remote in the fvwm3 repository and try and cherry-pick/rebase the relevant commits. But I suspect neither branches will have any common ancestry with the master branch, so that might not work as well. But you could solve that through using grafts if you really wanted to. I can't really remember where we got to with that work either -- I'm hoping this trip down memory lane might jog your memory more than it has mine. ;) HTH, Thomas [1]: https://github.com/ThomasAdam/mvwm
Re: [PATCH] (6) Man page changes -- second attempt
On Wed, Nov 17, 2021 at 02:35:32PM +0100, Dominik Vogt wrote: > On Tue, Nov 16, 2021 at 01:36:53AM +0100, Dominik Vogt wrote: > > This is the full set of patches for splitting the man page, to be > > applied to master. > > Second attempt. The style docs are not moved aound in the man > page. The .section files have been renamed to .ad because > asciidoc only evaluates "ifdef" in included files if they have a > known extesion. ".adoc" cannot be used because the pattern rules > in the Makefile.am would conflict. > > The patch stack has been reshuffled and patches have heen merged, > and two new ones on top have been added. Thanks, Dominik. This applies cleanly now. I've also added an OVERVIEW section to fvwm3all.adoc explaining how the man page is split up into different sections. I'm going to merge this to master, albeit I'm hoping to create some additional sections as well so it doesn't feel like this change is incomplete. The PR is here: https://github.com/fvwmorg/fvwm3/pull/637 Kindly, Thomas
Re: [PATCH] (6) Man page changes -- second attempt
On Tue, Nov 16, 2021 at 01:36:53AM +0100, Dominik Vogt wrote:
> This is the full set of patches for splitting the man page, to be
> applied to master.
Second attempt. The style docs are not moved aound in the man
page. The .section files have been renamed to .ad because
asciidoc only evaluates "ifdef" in included files if they have a
known extesion. ".adoc" cannot be used because the pattern rules
in the Makefile.am would conflict.
The patch stack has been reshuffled and patches have heen merged,
and two new ones on top have been added.
0001: Man page split (only one patch now)
0002,3,4: General cleanup in man page.
0005: (new) Remove "open look" and "xview" from man page.
0006: (new) Remove "GlobalOpts" from man page, code and oteher files.
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt
From de749130845071272c41c34e84de2ebd8e6dc61d Mon Sep 17 00:00:00 2001
From: Dominik Vogt
Date: Mon, 15 Nov 2021 16:52:38 +0100
Subject: [PATCH 1/6] Split main man page.
fvwm3.1 basic documentation
fvwm3commands.1 command documentation
fvwm3all.1 all documentation
fvwm3menus.1 menu documentation
---
doc/.gitignore| 1 +
doc/{modules => }/FvwmAnimate.adoc| 0
doc/{modules => }/FvwmAuto.adoc | 0
doc/{modules => }/FvwmBacker.adoc | 0
doc/{modules => }/FvwmButtons.adoc| 0
doc/{bin => }/FvwmCommand.adoc| 0
doc/{modules => }/FvwmConsole.adoc| 0
doc/{modules => }/FvwmEvent.adoc | 0
doc/{modules => }/FvwmForm.adoc | 0
doc/{modules => }/FvwmIconMan.adoc| 0
doc/{modules => }/FvwmIdent.adoc | 0
doc/{modules => }/FvwmMFL.adoc| 0
doc/{modules => }/FvwmPager.adoc | 0
doc/{modules => }/FvwmPerl.adoc | 0
doc/{bin => }/FvwmPrompt.adoc | 0
doc/{modules => }/FvwmRearrange.adoc | 0
doc/{modules => }/FvwmScript.adoc | 0
doc/Makefile.am | 61 +-
doc/README| 8 -
doc/{bin => }/fvwm-config.adoc| 0
doc/{bin => }/fvwm-convert-2.6.adoc | 0
doc/{bin => }/fvwm-menu-desktop.adoc | 0
doc/{bin => }/fvwm-root.adoc | 0
doc/fvwm3.adoc| 7 +
.../fvwm3.adoc => fvwm3_manpage_source.adoc} | 545 +-
doc/fvwm3all.adoc | 7 +
doc/fvwm3commands.adoc| 7 +
doc/fvwm3menus.adoc | 7 +
doc/fvwm3styles.adoc | 7 +
29 files changed, 350 insertions(+), 300 deletions(-)
create mode 100644 doc/.gitignore
rename doc/{modules => }/FvwmAnimate.adoc (100%)
rename doc/{modules => }/FvwmAuto.adoc (100%)
rename doc/{modules => }/FvwmBacker.adoc (100%)
rename doc/{modules => }/FvwmButtons.adoc (100%)
rename doc/{bin => }/FvwmCommand.adoc (100%)
rename doc/{modules => }/FvwmConsole.adoc (100%)
rename doc/{modules => }/FvwmEvent.adoc (100%)
rename doc/{modules => }/FvwmForm.adoc (100%)
rename doc/{modules => }/FvwmIconMan.adoc (100%)
rename doc/{modules => }/FvwmIdent.adoc (100%)
rename doc/{modules => }/FvwmMFL.adoc (100%)
rename doc/{modules => }/FvwmPager.adoc (100%)
rename doc/{modules => }/FvwmPerl.adoc (100%)
rename doc/{bin => }/FvwmPrompt.adoc (100%)
rename doc/{modules => }/FvwmRearrange.adoc (100%)
rename doc/{modules => }/FvwmScript.adoc (100%)
rename doc/{bin => }/fvwm-config.adoc (100%)
rename doc/{bin => }/fvwm-convert-2.6.adoc (100%)
rename doc/{bin => }/fvwm-menu-desktop.adoc (100%)
rename doc/{bin => }/fvwm-root.adoc (100%)
create mode 100644 doc/fvwm3.adoc
rename doc/{fvwm3/fvwm3.adoc => fvwm3_manpage_source.adoc} (99%)
create mode 100644 doc/fvwm3all.adoc
create mode 100644 doc/fvwm3commands.adoc
create mode 100644 doc/fvwm3menus.adoc
create mode 100644 doc/fvwm3styles.adoc
diff --git a/doc/.gitignore b/doc/.gitignore
new file mode 100644
index ..38ff07fe
--- /dev/null
+++ b/doc/.gitignore
@@ -0,0 +1 @@
+/*.ad
diff --git a/doc/modules/FvwmAnimate.adoc b/doc/FvwmAnimate.adoc
similarity index 100%
rename from doc/modules/FvwmAnimate.adoc
rename to doc/FvwmAnimate.adoc
diff --git a/doc/modules/FvwmAuto.adoc b/doc/FvwmAuto.adoc
similarity index 100%
rename from doc/modules/FvwmAuto.adoc
rename to doc/FvwmAuto.adoc
diff --git a/doc/modules/FvwmBacker.adoc b/doc/FvwmBacker.adoc
similarity index 100%
rename from doc/modules/FvwmBacker.adoc
rename to doc/FvwmBacker.adoc
diff --git a/doc/modules/FvwmButtons.adoc b/doc/FvwmButtons.adoc
similarity index 100%
rename from doc/modules/FvwmButtons.adoc
rename to doc/FvwmButtons.adoc
diff --git a/doc/bin/FvwmCommand.adoc b/doc/FvwmCommand.adoc
similarity index 100%
rename from doc/bin/FvwmCommand.adoc
rename to doc/FvwmCommand.adoc
diff --git a/doc/modules/FvwmConsole.adoc b/doc/FvwmConsole.adoc
similarity index 100%
Re: parser branch
On Wed, Nov 17, 2021 at 02:38:19PM +0100, Dominik Vogt wrote: > I'd like to finish the parser work started in 2014. Is the old > branch still available somewhere? Remind me what work that was... Kindly, Thomas
parser branch
I'd like to finish the parser work started in 2014. Is the old branch still available somewhere? Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [PATCH] (5) Man page changes
On Wed, Nov 17, 2021 at 05:59:49AM +, Thomas Adam wrote: > On Tue, Nov 16, 2021 at 01:36:53AM +0100, Dominik Vogt wrote: > > This is the full set of patches for splitting the man page, to be > > applied to master. > > > > 1, 2 and 4 are unrelated cleanups. > > 3 and 5 implement the split. > > > > 4 conflicts with both, 3 and 5, so it can't be pulled out of the > > sequence. > > I'm not able to apply these patches cleanly. Patch 5 is failing to apply: > > .git/rebase-apply/patch:6588: space before tab in indent. > StaysOnTop, WindowListSkip > .git/rebase-apply/patch:6591: space before tab in indent. > WindowListSkip > error: patch failed: doc/fvwm3_manpage_source.adoc:5186 Please apply them now while I see what can be done to fix #5. Can you give me the hook scripts that do these checks so that I catch these things locally before sending patches? > Any thoughts? Some of the examples are indented with tabs; something like this: --snip-- Examples: ``` foo bar baz ``` --snip-- I moved around a big chunk of text because there was some problem with nested ifdefs. If this can be solved it's not necessary to shuffle text blocks. Let me take a look. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
