On Wed, Nov 17, 2021 at 10:40:56PM +0000, 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

> 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
 * 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
 * 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.


Dominik ^_^  ^_^


Dominik Vogt

Reply via email to