Re: [RFC] New parser framework for testing
On Thu, Nov 18, 2021 at 03:31:46PM +, Thomas Adam wrote: > On Thu, Nov 18, 2021 at 02:19:11PM +0100, Dominik Vogt wrote: > > Most of the tests were meant to catch parsing bugs, leaks and > > crashes. A mor organised approach in the future would be good. > > Maybe it would even be possible to generate test cases for > > commands programmatically from the BNF. > > It might be -- although my faith in our accuracy of the BNF notation we came > up with isn't too strong. Of course - the (long term) goal should also be to change the syntax of unmanageable commands so that they can be handled in BNF without much trouble. > Either way, I'll put something together which will > make it easy to expand. Thanks. > I think you're right though, Dominik, we need to pull this into a .md file and > start fleshing it out (similar to how we did the BNF work), if you're happy > for that? Indeed. > > Taking it a step further filters can be applied to *any* command > > line, not just commands: > > > > foobarfunc --match-resource "xterm" > > > > (Problem: How can we distinguish between general filters and the > > actual command/function arguments?) > > If a command in a complex function isn't overriding the calling filter, then > use that? I mean regarding parsing. If filters are universal, they shouldn't be part of the command syntax, and the parser needs to figure out which arguments are filters and which belong to the command, e.g. by a rule that filter arguments must always be placed before other arguments on the command line. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [RFC] New parser framework for testing
On Thu, Nov 18, 2021 at 02:19:11PM +0100, Dominik Vogt wrote: > Most of the tests were meant to catch parsing bugs, leaks and > crashes. A mor organised approach in the future would be good. > Maybe it would even be possible to generate test cases for > commands programmatically from the BNF. It might be -- although my faith in our accuracy of the BNF notation we came up with isn't too strong. Either way, I'll put something together which will make it easy to expand. > Sounds interesting. While implementing new ways of selecting > source/target (what is the difference?) it is still possible to > keep the existing conditionals working: If "-s ..." is present, > use it. Otherwise use the window that has been selected by a > conditional. If that's not defined either, ask the user to select > one. Yes, in that order as well, I would have thought. I'm still mulling over what I mean by source vs target, but source refers to the window or windows which the command needs to operate on, and target is the end result of that command. Take the `Move` command, for instance: Move -s -t screen|desktop|page But I think that's overcomplicating things. What matters is whether the command expects a window, or if it's a command which changes state (such as a colorset, or a setting for something). I think you're right though, Dominik, we need to pull this into a .md file and start fleshing it out (similar to how we did the BNF work), if you're happy for that? > For multi-target conditions the syntax would work too. > > Old way, loop over all windows, filter them by a resource name, > then apply a command to them: > > All (xterm) Close > > Same in new syntax (assuming "-c" marks the beginning of the > command to execute): > > All --match-resource "xterm" -c Close Something like that, yes. Although I'm suggesting that filters would allow for us doing away with commands such as: All, None, Next, Prev, as they'd become a part of the filter. So you might say: Close --filter "screen:X,desk:*,class:XTerm,condition:*" ... to close all XTerm windows (condition:*), which were only on screen X, all desks (desk:*). Either way would work though. > Taking it a step further filters can be applied to *any* command > line, not just commands: > > foobarfunc --match-resource "xterm" > > (Problem: How can we distinguish between general filters and the > actual command/function arguments?) If a command in a complex function isn't overriding the calling filter, then use that? > Note: Complex functions already have a kind of filtering with the > "I", "C", ... bits. Yeah -- this needs more thinking. Hmm. Kindly, Thomas
Re: [RFC] New parser framework for testing
We need to put the results and suggestions of this discusstion in a file. > On Thu, Nov 18, 2021 at 12:31:09AM +0100, Dominik Vogt wrote: > > Anyway, we need > > infrastructure for automated testing. > > We used to have something like that but it fell into bitrot and I removed it > years ago. Yep. > That being said, it's probably more valuable to have a set of > tests which capture the behaviour of the parser, than it is about checking > window positions, etc. Most of the tests were meant to catch parsing bugs, leaks and crashes. A mor organised approach in the future would be good. Maybe it would even be possible to generate test cases for commands programmatically from the BNF. > I see some of this as recognising that the commands need to have a common > syntax. Just dreaming up something here, but take the Move command for > example: > >Move <-- context is known or asked for, but interactive nonetheless >Move -s fvwm.next.XTerm <-- next XTerm in the ring (but interactive) >Move -s fvwm.prev.XTerm -p 200p 100p <-- prev XTerm, non-interactive > > (Here, -s indicates the *source* window). > > Resize could also work the same with with -s >... > Commands might collectively take '-s' to indicate a source, or '-t' to > indicate the destination. Be it a specific geometry, pixel/percentage, > desktop, page, etc. The syntax for these can be unified and abstracted away. >... Sounds interesting. While implementing new ways of selecting source/target (what is the difference?) it is still possible to keep the existing conditionals working: If "-s ..." is present, use it. Otherwise use the window that has been selected by a conditional. If that's not defined either, ask the user to select one. For multi-target conditions the syntax would work too. Old way, loop over all windows, filter them by a resource name, then apply a command to them: All (xterm) Close Same in new syntax (assuming "-c" marks the beginning of the command to execute): All --match-resource "xterm" -c Close Hypothetical filtering by the command itself. Call the command for each window in turn, but the command does nothing unless the --match-resource condition is true. All -c Close --match-resource "xterm" Command that works only on matching windows: Close --match-resource "xterm" etc. "All" would then work like a prefix (a la "silent", "keeprc" etc.). -- Taking it a step further filters can be applied to *any* command line, not just commands: foobarfunc --match-resource "xterm" (Problem: How can we distinguish between general filters and the actual command/function arguments?) Note: Complex functions already have a kind of filtering with the "I", "C", ... bits. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [RFC] New parser framework for testing
On Thu, Nov 18, 2021 at 12:31:09AM +0100, Dominik Vogt wrote:
> 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.
We used to have something like that but it fell into bitrot and I removed it
years ago. That being said, it's probably more valuable to have a set of
tests which capture the behaviour of the parser, than it is about checking
window positions, etc.
> 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.
Sure.
> 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.
Indeed -- and I understood that. My point was to try and pre-enumerate this
along with the window which is required to run the action. That works fine
for individual commands but each item in a function could be working on a
different context entirely. I think what I'm trying to convey here is two
different things which I shouldn't conflate.
> 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.
OK.
> 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.
I see this as the fundamental underpinings of moving things forward. Get this
right (or at the very least *standardised*), and everything becomes a lot
easier.
I see some of this as recognising that the commands need to have a common
syntax. Just dreaming up something here, but take the Move command for
example:
Move <-- context is known or asked for, but interactive nonetheless
Move -s fvwm.next.XTerm <-- next XTerm in the ring (but interactive)
Move -s fvwm.prev.XTerm -p 200p 100p <-- prev XTerm, non-interactive
(Here, -s indicates the *source* window).
Resize could also work the same with with -s
What I'm really talking myself into here is a DSL which represents the ability
to consistently apply filters to commands which operate consistently. This
would also mean conditional commands are not a separate entity, they're just
filters operating on other commands.
Commands might collectively take '-s' to indicate a source, or '-t' to
indicate the destination. Be it a specific geometry, pixel/percentage,
desktop, page, etc. The syntax for these can be unified and abstracted away.
So I think the BNF descriptions as we have them now are very useful and needed
to understand how the different commands behave now, and overlap. But for any
future changes, we need to think differently.
I don't like the overlap we have now between commands operating as
state/settings changed, and commands which all do the same thing as one
another. The BNF screams that to me even more than the man page does at this
point. ;P
> * Some way to store it in the function table (at compile time
>without making functable.h depend on all other .h files.)
Yes.
> * 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?).
We could start adding to the function table flags to indicate deprecation,
overlaps_with, etc. Along with parsing hints. For example (and completely
made up): imagine some command struct:
struct fvwm_command {
const char *cmd_name;
const char *syntax;
const char *other_cmd; <-- the alternate command to use if deprecated
int flags; <-- deprecated
context_t *context; <-- the window to operate on, etc.
};
So here, what I'm trying to convey is capturing the state of the command we
have now, whether it's deprecated (and the command to use instead), and some
context_t which has evaluated which
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
[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 =
