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

Extract self-contained components from crystal i #11177

Closed
3 of 4 tasks
straight-shoota opened this issue Sep 6, 2021 · 10 comments
Closed
3 of 4 tasks

Extract self-contained components from crystal i #11177

straight-shoota opened this issue Sep 6, 2021 · 10 comments

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Sep 6, 2021

The big crystal i branch (#11159) contains a number of auxiliary components which are used and required for building that, but are not integral parts of the compiler or interpreter proper. They're individual features that can stand on their own. As part of the main feature branch, they're just a means to an end, but I think they might deserve some focus on themselves. Like some isolated tests 😄
Some could even become useful additions to the standard library.

@caspiano
Copy link
Contributor

caspiano commented Sep 7, 2021

Would this be worth making into a checklist for better tracking?

@vlazar
Copy link
Contributor

vlazar commented Sep 7, 2021

@straight-shoota FYI LibFFI bindings and LibReadline bindings in description are incorrectly linked to SyntaxHighlighter.

@asterite
Copy link
Member

asterite commented Sep 7, 2021

This sounds good, but I don't agree at all with it, sorry.

The way I use libffi for the interpreter, I set a memory region where I put values there and perform the call. A proper interface for libffi would be very different, for example maybe you can pass arguments to a call using a splat, and some type checks would be done. I don't know. In Ruby there's fiddle, we can take some inspiration from there.

But I don't understand why we need to extract this as a separate component. We did that with markdown in the past, and eventually had to remove it and put it as private.

I really don't want to bother right now thinking about public APIs for those things.

Same goes for libreadine, dlopen and the syntax highligther.

Well, unless I'm misunderstanding this issue. Maybe it's something that could be done in parallel with the interpreter. But note that, at least for libffi, the interpreter will not use the public API because the public API will likely do some extra checks.

@straight-shoota
Copy link
Member Author

straight-shoota commented Sep 7, 2021

@asterite The intention is not necessarily to make all those APIs be public parts of the standard library. I explicitly mentioned this could apply to some of them if they're universally usable.

The main motivation is to look at those components individually and make sure they are well-defined and work as intended. Even if they stay internal libraries for the compiler. In the big interpreter PR they're missing attention. I expect focusing on self-contained pieces makes it easier to review and improve the entire thing. It's divide and conquer. We can easily separate these libraries from the rest of the interpreter code and test them in isolation for example.

@straight-shoota
Copy link
Member Author

For readline in particular, there is already a shard https://github.com/crystal-lang/crystal-readline which used to be part of the standard library (#8364). We can at least pull that shard into the repository, just like we did with markd (#11040). Alternatively, we could consider to revert the extraction and add it back to std-lib. But I think vendoring in is quite a neat solution.
In any case, both options avoid unnecessary code duplication.
Also in any case, we must give it some love, docs and specs. It's nice that it appears to mostly work as expected. But if it is to be used as part of the compiler, it should meet the expectations about quality and verification for compiler and stdlib code.

@asterite
Copy link
Member

asterite commented Sep 8, 2021

Those bindings for readline are wrong, at least for autocomplete. So I wouldn't por them over just like that.

@straight-shoota
Copy link
Member Author

Crystal::Repl::SyntaxHighlighter could be merged with Crystal::Doc::Highlighter, which it appears to be derived from. They're both almost identical and just differ in the presentation frontends, ANSI codes and HTML.

I think this could be worth exposing in the standard library. It might not be a super common feature, but it's closely tied to the language itself, which suggests a place in stdlib (as opposed to an external shard). I can certainly think of use cases where highlighting Crystal code in user applications might be handy.

@straight-shoota
Copy link
Member Author

@asterite Yeah, that's what I mean with giving some love and making sure it works as expected.

@asterite
Copy link
Member

asterite commented Sep 9, 2021

In the big interpreter PR they're missing attention. I expect focusing on self-contained pieces makes it easier to review and improve the entire thing. It's divide and conquer. We can easily separate these libraries from the rest of the interpreter code and test them in isolation for example.

Sounds good, but be aware that I don't plan to do that work.

@straight-shoota
Copy link
Member Author

I'm closing this. The interpreter has been merged into master.
Only readline is missing which is tracked in #11340. No reason to keep this open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants