wisecrack repl patches attached

[Jason E. Aten]

Dear Michael, Shannon, and Crack-language enthusiasts everywhere,

I haven't had a much time to work on wisecrack, and I don't know how much time I'll have in the future; but I didn't want the repl progress be lost.  Wisecrack is great fun to work in, and it really shows off the language nicely.

I took care of the last few things on the todo list, put it in a reasonable state, and wanted to convey its state and code to you.  I merged in the latest changes from Shannon's github mirror (as of 5 Feb 2013). It should be good to go.

For reference, I generated the attached patch file with:

git clone https://github.com/glycerine/crack-language.git
git checkout editlinebuild
git remote add shannon   https://github.com/weyrick/crack-language.git
git fetch shannon
git diff shannon/master > editline.build.patches
gzip editline.build.patches


Release notes:

1. libedit is included in the distribution under wisecrack/libedit/, so that its functionality is always available for the repl. Rationale: once someone gets used to the repl, it makes no sense to deprive them of up-arrow for command history and the other nice things that libedit provides. I updated the README to note the BSD license for libedit.

2. Theory of repl operation: syntax errors at the repl generate namespace entries and partial code generations that must be transactionally rolled-back. The model/OrderedIdLog class provides the mechanism to rollback the namespace additions. The Builder::purgeUnterminatedFunctions() function takes care of the second part, deleting parital functions that would otherwise cause subsequent codegen problems.


Enjoy!

- Jason

[Michael Muller]

Jason E. Aten wrote:
> Dear Michael, Shannon, and Crack-language enthusiasts everywhere,
>
> I haven't had a much time to work on wisecrack, and I don't know how much
> time I'll have in the future; but I didn't want the repl progress be lost.
> Wisecrack is great fun to work in, and it really shows off the language
> nicely.

Jason, thanks so much for doing this. I apologize for not giving your changes
the attention that they deserve so far, I've been kind of wrapped up in the
caching work that's been happening since before you began the wisecrack work.
Also, to be frank, it's difficult to digest changes that come in very big
batches like these have. Smaller, more focused changes towards a bigger goal
are far more likely to make it into the main repo in a timely manner.

But in any case, I promise to have a look at your diffs again soon.

>
> I took care of the last few things on the todo list, put it in a reasonable
> state, and wanted to convey its state and code to you. I merged in the
> latest changes from Shannon's github mirror (as of 5 Feb 2013). It should
> be good to go.
>
> For reference, I generated the attached patch file with:
>
> git clone https://github.com/glycerine/crack-language.git
> git checkout editlinebuild
> git remote add shannon https://github.com/weyrick/crack-language.git
> git fetch shannon
> git diff shannon/master > editline.build.patches
> gzip editline.build.patches
>
>
> Release notes:
>
> 1. libedit is included in the distribution under wisecrack/libedit/, so
> that its functionality is always available for the repl. Rationale: once
> someone gets used to the repl, it makes no sense to deprive them of
> up-arrow for command history and the other nice things that libedit
> provides. I updated the README to note the BSD license for libedit.

I've never been happy about the cases where we add external files to the
source tree. This approach should be reserved for cases where there is no
external package, which isn't true for libedit.

I understand not wanting to deprive users of that functionality, but we have
other dependencies that are no less essential (the regular expression module,
for example) which are optional and only built if the associated dev packages
are installed.

This dependency really needs to be triggered by a configuration test.

>
> 2. Theory of repl operation: syntax errors at the repl generate namespace
> entries and partial code generations that must be transactionally
> rolled-back. The model/OrderedIdLog class provides the mechanism to
> rollback the namespace additions. The Builder::purgeUnterminatedFunctions()
> function takes care of the second part, deleting parital functions that
> would otherwise cause subsequent codegen problems.
>
>
> Enjoy!
>
> - Jason
>

=============================================================================
michaelMuller = mmu...@enduden.com | http://www.mindhog.net/~mmuller
-----------------------------------------------------------------------------
There is no way to find the best design except to try out as many designs as
possible and discard the failures. - Freeman Dyson
=============================================================================

[Michael Muller]

So I've spent a couple of hours looking at this. I mostly like the code, and
I appreciate the attention to coding style in it. Here are my outstanding
criticisms:

- (as stated earlier) I don't want us to check in libedit.
- We need to move TokerMsg to Toker::Msg (toplevel classes should be in
their own header files). Also, since TokerMsg seems to be something that
ends up getting passed around all over the place, we should move this
state information into Toker.
- Use the (fairly recent) tracing infrastructure instead of printf (see
model/Serializer.h's "trace" variable and crack_main.cc's initialization
of the option. Also, trace to cerr.
- I think LLVMBuilder::eraseSection() can still leave functions
dangling because crack allows nested functions, so if you did
void f() { void g() { <erase>
I think f() would be left dangling.
- There's LLVM code in purgeUnterminatedFunctions that's clearly lifted from
LLVM's verifier. It needs to be either rewritten or moved to another
module under the original licensing, preferably the former.
- I still think that module sections can be accomplished with fewer
additions to the builder.
- I noticed some test code in there (good!) which should really be moved
into the new unit test binary or something like it. Also, I didn't notice
any broader system tests for the REPL, we'll want to do this somehow
so we can tell if we break any of these features.

I started going through and doing a more detailed review, but it sounds like
you may not have time to work on this right now. As it stands, I don't have
time to work on it right now either. So I think I'm just going to hold onto
these changes and look into integrating them either before the 1.0 release,
but after I deal with everything else.

Thanks for your patience, I promise you that some form of this will get
integrated upstream at some time.

> --
> You received this message because you are subscribed to the Google Groups "crack-lang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to crack-lang-de...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

>
>


=============================================================================
michaelMuller = mmu...@enduden.com | http://www.mindhog.net/~mmuller
-----------------------------------------------------------------------------

Lokah Samasta Sukhino Bhavantu - May all beings everywhere be happy and free.
And may my own thoughts and actions contribute to that happiness and freedom.
=============================================================================

[Jason E. Aten]

Thanks Michael.  I'll add some clarifying comments below.

On Tue, Feb 12, 2013 at 8:19 AM, Michael Muller <mmu...@enduden.com> wrote:

-   (as stated earlier) I don't want us to check in libedit.

Conrad did the cmake version of this; which can be easily commented back in.  The autoconf side would still need to be done.  Since the autoconf side isn't done, the build would be broken without the current inclusion.
 

-   We need to move TokerMsg to Toker::Msg (toplevel classes should be in
    their own header files).  Also, since TokerMsg seems to be something that
    ends up getting passed around all over the place, we should move this
    state information into Toker.

TokerMsg depends on the call stack for its semantics (pushing/popping depending on parser state), so putting it into the Toker wouldn't be right.  It is really a message *from* the parser *to* the toker, telling the toker about the parser's current state.
 

-   I think LLVMBuilder::eraseSection() can still leave functions
    dangling because crack allows nested functions, so if you did
        void f() { void g() { <erase>
    I think f() would be left dangling.

Hmm. Since this seemed surprising to me, I checked that just now. I don't see any dangling code that isn't cleaned up; using .dc to display code.
 

-   There's LLVM code in purgeUnterminatedFunctions that's clearly lifted from
    LLVM's verifier.  It needs to be either rewritten or moved to another
    module under the original licensing, preferably the former. 

For anyone who has time to do this in the future, this would simply mean changing builder/llvm/LLVMBuilder.cc:2322

from

-        for (Function::iterator I = F.begin(), E = F.end(); I != E; ++I) {
-            if (I->empty() || !I->back().isTerminator()) {
-                // std::cerr << "Basic Block in function '" <<
-                // F.getName().str() << "' does not have terminator!"
-                // << " doing auto purge to recover.\n";
-

to

+        for (Function::iterator iter = F.begin(), eIt = F.end(); iter != eIt; ++iter) {
+            if (iter->empty() || !iter->back().isTerminator()) {


The rest of the code is original.  The comments preceeding the function provide further clarification.


Cheers,
Jason

[Michael Muller]

Jason E. Aten wrote:
> Thanks Michael. I'll add some clarifying comments below.
>
> On Tue, Feb 12, 2013 at 8:19 AM, Michael Muller <mmu...@enduden.com> wrote:
>
> - (as stated earlier) I don't want us to check in libedit.
> >
>
> Conrad did the cmake version of this; which can be easily commented back
> in. The autoconf side would still need to be done. Since the autoconf
> side isn't done, the build would be broken without the current inclusion.
>
>
> > - We need to move TokerMsg to Toker::Msg (toplevel classes should be in
> > their own header files). Also, since TokerMsg seems to be something
> > that
> > ends up getting passed around all over the place, we should move this
> > state information into Toker.
> >
>
> TokerMsg depends on the call stack for its semantics (pushing/popping
> depending on parser state), so putting it into the Toker wouldn't be
> right. It is really a message *from* the parser *to* the toker, telling
> the toker about the parser's current state.

It looks like you basically want to set the "nested" flag during a
specific scope and restore it when you leave that scope. We do that sort
of thing in a number of places in the codebase using sentinels. You could
define a sentinel class like this:

struct TokerScope {
Toker &toker;
bool oldState;
TokerScope(Toker &toker) : toker(toker), oldState(toker.getNested()) {
toker.setNested(true);
}

~TokerScope() {
toker.setNested(oldState);
}
};

And then in the contexts where you want to turn on "nested":

TokerScope tokerScope(toker);

From what I could see, this would save you from having to pass this
structure around a few hundred times. It's a net win in terms of both
performance and readability :-)

=============================================================================
michaelMuller = mmu...@enduden.com | http://www.mindhog.net/~mmuller
-----------------------------------------------------------------------------

Government is not reason, it is not eloquence; it is force. Like fire, it
is a dangerous servant and a fearsome master. - George Washington
=============================================================================