monome / teletype

monome eurorack module
GNU General Public License v2.0
195 stars 82 forks source link

PREs are allowed without MODs #72

Closed samdoshi closed 7 years ago

samdoshi commented 7 years ago

As far as I can tell this bug has been there since the beginning. Or at least for quite a long time.

Consider the following

TR.PULSE 1 : TR.PULSE 2

That command doesn't make any sense as the PRE (i.e. before the :) has no MOD statement (e.g. IF). But it passes validation and runs!

What happens is the parser splits the entire command into a PRE and a POST, the validation step doesn't check to see if the first WORD of the PRE is a MOD. And finally the processor only executes the PRE (assuming that execution of the POST is delegated to the non-existent MOD).

The fix is fairly straightforward, we just need to insert a check at the end of validate to assert that if there is a PRE it must start with a MOD.

Unless I hear any objections I'll get it fixed in my parser branch.

tehn commented 7 years ago

thank you for spotting this, and the fix sounds correct.

On Mon, Mar 6, 2017 at 9:24 AM, Sam Doshi notifications@github.com wrote:

As far as I can tell this bug has been there since the beginning. Or at least for quite a long time.

Consider the following

TR.PULSE 1 : TR.PULSE 2

That command doesn't make any sense as the PRE (i.e. before the :) has no MOD statement (e.g. IF). But it passes validation and runs!

What happens is the parser splits the entire command into a PRE and a POST, the validation step doesn't check to see if the first WORD of the PRE is a MOD. And finally the processor only executes the PRE (assuming that execution of the POST is delegated to the non-existent MOD).

The fix is fairly straightforward, we just need to insert a check at the end of validate to assert that if there is a PRE it must start with a MOD.

Unless I hear any objections I'll get it fixed in my parser branch.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/monome/teletype/issues/72, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPEcBG8KN6fsHaQEkvIV094IFqg4lxNks5rjBc4gaJpZM4MUNiO .

samdoshi commented 7 years ago

Fixed here:

https://github.com/samdoshi/teletype/commit/e70ec4bf4eb11058f7c829706521c1e3979eb812

samdoshi commented 7 years ago

Fixed in #73