ssj71 / OSC2MIDI

A highly flexible and configurable OSC to JACK MIDI (and back) bridge
GNU General Public License v3.0
45 stars 7 forks source link

improved syntax checking of map files #5

Closed agraef closed 9 years ago

agraef commented 9 years ago

Even after the parser improvements in my previous pull request osc2midi would still happily accept some bogus lines such as:

/2/multipush1/2/2 f, val : controlchange( 0, 5, 127*

Therefore I decided to write an improved parser which is in this pull request. It is implemented as a separate config_check() routine which is called to check the syntax beforehand, do that well and do nothing else. The rest of the parsing is done as before, so there's no change of code in the other semantic routines which do the actual processing.

config_check() implements a full-blown parser for omm rules, so it checks for syntax errors much more reliably than the bunch of scanf's which is used in the semantic routines. (See the comments next to the routine for the EBNF grammar which is implemented.)

A regression test is included as maps/syntax.omm. This is not a working map file. It's full of syntax errors and is not intended to be used for any purpose other than a regression test for the osc2midi parser. A proper implementation should find an error in each of the rules in this file, and check_config() does. On the other hand, it also parses and accepts all the other maps included with osc2midi as well as my own maps distributed with osc2midi-utils.

I've also added a new "dry run" option -n which lets me run syntax checks more conveniently and see the number of detected errors at a glace. It is used like this:

osc2midi -n -m syntax.omm

Note: The parser contains non-local exits (even gotos) galore (mostly error exits), but I consider these customary style when writing parsers. ;-)

agraef commented 9 years ago

Just for the record, here's what osc2midi -n gives when run on syntax.omm. Note that the exact position of each syntax error is marked with three carets ^^^, which is supposed to make it easy for the user to figure out what needs changing.

ERROR in config line:
/1, : noteon( 0, 60, 127 );
    ^^^
 -syntax error: expected ','

ERROR in config line:
/1 : noteon( 0, 60, 127 );
   ^^^
 -syntax error: expected ','

ERROR in config line:
/1: noteon( 0, 60, 127 );
    ^^^
 -syntax error: unknown OSC type

ERROR in config line:
/1/fader1 f, 1- : controlchange( 0, 1, 127*val );
                ^^^
 -syntax error: expected variable

ERROR in config line:
/1/fader1 f, val- : controlchange( 0, 1, 127*val );
                  ^^^
 -syntax error: expected number

ERROR in config line:
/1/fader1 f, +val : controlchange( 0, 1, 127*val );
             ^^^
 -syntax error: expected variable

ERROR in config line:
/1/fader2 f, val : controlchange( 0, 1-, 127*val );
                                       ^^^
 -syntax error: expected variable

ERROR in config line:
/1/fader2 f, val : controlchange( 0, 1, 127* );
                                             ^^^
 -syntax error: expected variable

ERROR in config line:
/1/fader2 f, val : controlchange( 0, 1, *val );
                                        ^^^
 -syntax error: expected variable

ERROR in config line:
/2 , : noteon( 0, 61, 127+ );
                           ^^^
 -syntax error: expected variable

ERROR in config line:
/2/multipush1/1/1 f, val : controlchange( );
                                          ^^^
 -syntax error: expected midi arguments

ERROR in config line:
/2/multipush1/1/1 f, val : controlchange
                                        ^^^
 -syntax error: expected '('

ERROR in config line:
/2/multipush1/2/1 f, val : controlchange( 0, , 127*val );
                                             ^^^
 -syntax error: expected midi argument

ERROR in config line:
/2/multipush1/3/1 f, val : controlchange( 0, 1-3, 127*val );
                                                ^^^
 -syntax error: range is not allowed here

ERROR in config line:
/2/multipush1/1/2 f, val controlchange( 0, 4, 127*val );
                         ^^^
 -syntax error: expected ',' or ':'

ERROR in config line:
/2/multipush1/2/2 f, val : controlchange( 0, 5, 127* 
                                                     ^^^
 -syntax error: expected variable

ERROR in config line:
/2/multipush1/3/2 f, val : controlchange( 0, 6, 127*val ); junk
                                                           ^^^
 -syntax error: expected end of line or comment

ERROR in config line:
/2/xy1 ff, 5+-3,  : controlchange( 1, 1, 127*val );
               ^^^
 -syntax error: expected '*'

ERROR in config line:
/2/xy1 ff, , *-3 : controlchange( 1, 2, 127*val );
             ^^^
 -syntax error: expected variable

Found 19 error(s), exiting
ssj71 commented 9 years ago

sounds like a great idea. thanks

Just curious why not a -d flag for dry run?

agraef commented 9 years ago

Just curious why not a -d flag for dry run?

Mercurial uses -n as its shorthand for --dry-run. Guess it's another one of those idiosyncrasies which stick after a while. :) Just change it to whatever you like (makes sense to keep it short, though).

ssj71 commented 9 years ago

I don't know which will be better for most users. I figured it was that way in some other program. We can leave it as it is for now.