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