Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experemental interoperation improvements #25

Conversation

Drainful
Copy link
Contributor

@Drainful Drainful commented Jan 28, 2019

Fixes #24

This is an attempt at easing interoperability between Shen and Lisp. I began by putting all Lisp code (including the output of translated K Lambda, and the top level REPL) into a separate package :SHEN. This allows external Lisp libraries to be run without accidental intersection with Shen names. Secondly, I created three functions (located in native.lsp) to more easily call Lisp code from Shen.

  • load-lisp, a function which loads the given Common Lisp file in the COMMON-LISP-USER package and in the :UPCASE readtable case.
  • load-inline-lisp, which loads Lisp from a string as an input stream under the same conditions as load-lisp.
  • eval-inline-lisp, which evaluates a lisp form from a string and returns its value under the same conditions as load-lisp.

I believe that these changes will vastly improve the ability of Shen to interoperate with Common Lisp code. Please advise me on what changes if any I ought to make to this pull request.

Thank you.

@tizoc
Copy link
Member

tizoc commented Jan 28, 2019

Thank you @Drainful!

Overall this looks good to me (but lets see what @rkoeninger has to say, he is more familiar than me with both Common Lisp and this port).

Something I would change: prefix the new functions with shen-cl. since they are extensions specific to this port, and we want to keep the global namespace clean.

@Drainful
Copy link
Contributor Author

Drainful commented Jan 28, 2019

That's a good idea. I have renamed them.

Also, I had only tested with SBCL earlier, but now I realize that on my machine the CLISP implementation fails tests on chapter 13 with a segmentation fault on both my fork and the master branch, while the ECL implementation fails only on my fork while attempting to load the tests, saying that the :SHEN package does not exist. I will try to resolve these issues. CCL works fine.

@rkoeninger
Copy link
Member

@Drainful CLisp tests fail on master? Should probably open a separate issue for that including the specs of your machine so we can try to reproduce it.

@Drainful
Copy link
Contributor Author

I created the issue: #26

@rkoeninger
Copy link
Member

@tizoc do travis-ci builds not trigger for PRs in this repo?

@tizoc
Copy link
Member

tizoc commented Jan 28, 2019

@rkoeninger no idea what is going on, but I checked the logs here https://travis-ci.org/Shen-Language/shen-cl/requests

and it says "abuse detected".

Any idea what that means? could the test be taking too much time or using too much resources?

@Drainful
Copy link
Contributor Author

I don't think the abuse metrics are made public but that seems possible. I ought to close this PR until I get the bugs sorted out.

@Drainful Drainful closed this Jan 28, 2019
@tizoc
Copy link
Member

tizoc commented Jan 28, 2019

@Drainful I found this https://travis-ci.community/t/some-pr-are-not-analyzed-with-message-abuse-detected/1191

It seems that you registering and logging in to travis-ci.org (if you haven't ever done so in the past) would make you a "trusted" user to their system.

@Drainful
Copy link
Contributor Author

Drainful commented Jan 28, 2019

Alright I registered with this github account. I'll try re-opening to trigger a travis build.

EDIT: I have received the following notification on travis "The account Ipkcle has been flagged for potential abuse. Please contact support@travis-ci.com.". Either there is something else about the PR triggering the abuse warning, or perhaps my account was too new. I'll contact them.

@Drainful Drainful reopened this Jan 28, 2019
@rkoeninger
Copy link
Member

rkoeninger commented Jan 28, 2019

@Drainful push another commit to trigger a build

@rkoeninger
Copy link
Member

rkoeninger commented Jan 28, 2019

Hmmm... still saying abuse dectected. I'll try pushing a commit...

Doesn't like me either. Repo must be flagged.

@Drainful
Copy link
Contributor Author

I fixed the issue with ECL. The package definition, being in boot.lsp, was not compiled into into an object file for ECL's C:BUILD-PROGRAM. I moved it to a separate file in the src directory. Travis looks good too.

@rkoeninger
Copy link
Member

Trying it out on my machine and it works as advertised.

It's a little awkward to work between packages though: if I define a function in :SHEN, I can't reference it from shen-cl.eval-inline-lisp (qualified with SHEN::) nor can I reference it from lisp. since both uppercase the name of the function and Shen functions aren't going to be named in all uppercase.

Ideally, we'd be able to do casing on a per-namespace basis, not on a per-evaluation-method basis.

Can we make something like that work? Instead of upper-casing entire expressions, can we qualify specific symbols (those starting with SHEN::) as not getting upper-cased and uppercase everything else? This would be possible to do on the expressions passed our interop functions, but what about code that they load and evaluate?

@rkoeninger
Copy link
Member

Also, are both shen-cl.load-inline-lisp and shen-cl.eval-inline-lisp needed? They seem to be similarly capable but the eval- function returns the result. Could they be replaced with a single shen-cl.eval-lisp function?

@Drainful
Copy link
Contributor Author

Drainful commented Jan 29, 2019

As far as the justification for both load and eval, load allows you to treat the string as a separate file, including the ability to (in-package :shen), and has the equivalent of an implicit progn, while eval evaluates a single form. I agree that there is some redundancy though, perhaps load-lisp is sufficient for the first purpose.

The idea to evaluate shen:: symbols in a separate readcase is a good one. Current workarounds for the issues you mentioned would be to either call only capitalized Shen functions, or to manually set the readtable case yourself in common lisp code. It is not ideal though. I'll try to get per-namespace casing working.

As it stands the lisp. form is ideal for using common lisp code in the :shen package and :preserve casing (to easily access Shen functions), while eval-inline-lisp is useful for quickly accessing external lisp libraries which can only be used in :upcase (at the expense of making accessing Shen more difficult)

EDIT: Also I am not sure how to escape the " character in Shen strings, which makes referring to strings in inline lisp code difficult.

@rkoeninger
Copy link
Member

I don't know if doing casing on a per-package basis is feasible. I'm considering merging this as-is.

@tizoc thoughts?

@rkoeninger
Copy link
Member

@Drainful character escape sequence in Shen looks like c#, followed by the base-10 representation of the character code (actual character set is platform dependent), followed by a ;.

So the escape sequence for " is c#34;

@Drainful
Copy link
Contributor Author

I created a new package :SHEN-UTILS to contain code that is meant to be used from common lisp. It currently exports a function LOAD-SHEN to load a Shen file from lisp. I did not prefix it with shen-cl. because it is not accessible from Shen, and to use it from common lisp it would be prefixed like shen-utils:load-shen.

The only way to dynamically change casing within a file would be with a reader macro. I might get around to doing that eventually, but the current situation is workable. I would like to (slowly) begin work on creating a loadable ASDF system to allow the embedding of Shen code in programs with a Lisp entry point, though that should probably be a separate pull request.

@rkoeninger
Copy link
Member

rkoeninger commented Feb 3, 2019

@Drainful I'm going to go ahead and merge. Thanks for all your work on this!

@rkoeninger rkoeninger merged commit c718620 into Shen-Language:master Feb 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants