search

nikodemus / esrap

OLD REPOSITORY: Please go to:
https://github.com/scymtym/esrap
81 stars 25 forks source link

Hi, I've implemented extended dynamic wrapping, on which we takled earlier on email #22

Open mabragor opened 11 years ago

mabragor commented 11 years ago

Also, I fixed a bug, with infinite loop, which occured when greedy-repeating something that could match to an empty string. There are use-cases in tests.lisp and in examples-very-context-sensitive-grammar.lisp

Hopefully, this fix how will allow me to write YaML parser using esrap. But in case I missed something, I'll add fixes on the fly.

nikodemus commented 11 years ago

Thank you!

****AAARG. Effing markdown. Gimme plain text or give me death... so the numbers are screwed up, can't be arsed to figure out how to keep them right.

I took a quick look at this. VERY quick. Here are the issues I saw on my pass through the diff to origin/master:

  1. Extra dependencies are not acceptable, particularly not dependencies with viral licenses. (DEFMACRO-ENHANCE is GPL!), and really not wild about non-viral licences that are still more restrictive than esrap's own. (rutils -- use of strcat can also be replaced by alexandria:symbolicate.) Iterate is OK in principle (essentially the same licence as esrap), but not needed here, really.
  2. This is not confidence inspiring, and also very incomplete. When are FROM and TO evaluated? What do their values mean? I also mislike extending * like that. Too magical. Maybe (limit expr :from form :to form)) instead?
    • FROM and TO in (* ...) form may be arbitrary forms (e.g. special variables), but use with caution -
    • feature is experimental and probably does not handle local environment correctly.
  3. I don't like FIRST. I can see how it can be convenient, but destructuring should IMO remain separate from grammar.
  4. I like TAG, I think, though unsure about restricting it to keywords. Very nice!
  5. COND description in the README is unclear about what is consumed. If it is the way I assume it is, I think it's fine. Would like to see a use-case, though, which shows how it is more expressive than the alternatives. I like the idea, but haven't really thought it through yet, so examples would be nice.

  6. Junk left in:

    +(defun foo () nil) +

  7. Missing earmuffs, ditto with INDENT in the example. Definite no-go. (See http://random-state.net/log/3498750808.html for context, unless this is already familiar ground to you.):

    +(defparameter contexts nil) +(defmacro register-context (context-sym)

    • `(push ',context-sym contexts)) +

  8. Alexandria has hash-table-alist:

    +(defun hash->assoc (hash)

    • (iter (for (key val) in-hashtable hash)
    • (collect `(,key . ,val)))) +
  9. Removing the compiler-macro for PARSE is a show-stopper. It is essential for what I consider acceptable performance.
  10. Don't like the FROB in EVAL-EXPRESSION and COMPILE-EXPRESSION. Makes it harder to read with little expressive gain. Not a huge thing, but I'd rather not have it. While the CASE expressions are growing a bit too much, the next refactoring should be splitting them into DEFEXPRESSION macros or similar.
  11. Tabs in code. No biggie, but not preferred.
  12. Manual proper has not been updated. (doc/esrap.texinfo) I see :AROUND is missing from there as well. Oops... but let's not have the incompleteness creep up any more. :)
  13. I don't understand the point of -> and <-. Maybe if they had entries in the manual it might be more obvious. :)
  14. REGISTER-CONTEXT is exported but not documented.

I think I would like to see the different parts here separated out into a distinct pull requests. Now there are too many trees in the forest.

Thinking about how to integrate context sensitivity with packrat parsing in a more principled manner, I have a feeling that allowing users to define arbitary functions which could then function as terminals might be the way to go. See sketch on https://github.com/nikodemus/esrap/tree/custom-terminals-sketch (just pushed it). It seems to me that forcing all context into the same cache cannot really be a win, and correctness becomes also harder to reason about.

mabragor commented 11 years ago

OK, I'll try to fix the issues. But now, while integrating all those new syntactic constructs into COMPILE-EXPRESSION, I've noticed, that what's really done is duplicating some of the functionality of the Lisp code-walker.

So I decided to improve on HU.DWIM.WALKER a little bit, for it to be able to substitute unknown symbols with something else on the fly (to imitate ESRAP's behaviour, where all symbols are treated as non-terminals). Now my patch there is accepted, and I started to work on rewriting DEFRULE to use that code-walker for macroexpansion.

Of course, this allows to completely mix up destructuring with grammar, which you don't like.

I removed compiler macro for PARSE, since other way all the rules ended up in ESRAP::RULES hashtable not CL-YACLYAML::YACLYAML-RULES, which I wanted them to be in (because compiler macro works outside of dynamics, and my (LET ((ESRAP::RULES CL-YACLYAML::RULES)) simply had no effect on DEFRULE, which was a real pain to debug.

About separating into several pull requests, I know, how to do that in Darcs (where you can rewrite the commit history), but how to do that with Git?

nikodemus commented 11 years ago

On 22 June 2013 19:09, Alexandr Popolitov notifications@github.com wrote:

OK, I'll try to fix the issues. But now, while integrating all those new syntactic constructs into COMPILE-EXPRESSION, I've noticed, that what's really done is duplicating some of the functionality of the Lisp code-walker.

A code walker is a limited generalization of evaluator or compiler (or code analyzer), so obviously. :)

So I decided to improve on HU.DWIM.WALKER a little bit, for it to be able to substitute unknown symbols with something else on the fly (to imitate ESRAP's behaviour, where all symbols are treated as non-terminals). Now my patch there is accepted, and I started to work on rewriting DEFRULE to use that code-walker for macroexpansion.

I can right off say that a walker-using defrule won't be merged by me. ...but maintainership is probably moving to Jan or someone else soonish, so they may like it better. :)

I removed compiler macro for PARSE, since other way all the rules ended up in ESRAP::RULES

hashtable not CL-YACLYAML::YACLYAML-RULES, which I wanted them to be in (because compiler macro works outside of dynamics, and my (LET ((ESRAP::RULES CL-YACLYAML::RULES)) simply had no effect on DEFRULE, which was a real pain to debug.

Fine for your local usage, perhaps, but definitely not something to be merged.

About separating into several pull requests, I know, how to do that in Darcs (where you can rewrite the commit history), but how to do that with Git?

Easiest way is probably to make several branches, each with single feature -- and merge them into your all-features branch. Obviously if some things depend on each other, then a they cannot be separated into individual branches - but the thing depended on can still have it's own branch.

(Despite my today's pass over esrap pull requests, I'm on vacation and spending as much time away from the computer as i can -- and once I get back to work my time will be very short. ...which is why I'm hoping to pass on Esrap's maintainership to eg. Jan.)