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

Plugin API to prepare for frontend implemented in julia #32063

Closed
wants to merge 2 commits into from

Conversation

c42f
Copy link
Member

@c42f c42f commented May 17, 2019

I thought I'd try to get the first logical part of #31954 in to make it easier to iterate on separating the frontend.

There's two commits here, the first is pure code movement. The second is @Keno's work to create jl_frontend_t, with some additional minor cleanups (eg, make the new jl_fl_* functions static).

@c42f c42f requested a review from vtjnash May 17, 2019 08:27
@c42f c42f changed the title Plugin API to prepare for julia frontend Plugin API to prepare for frontend implemented in julia May 17, 2019
@vtjnash
Copy link
Member

vtjnash commented May 17, 2019

It appears the first commit is simply renaming the file but leaving behind a couple functions? It seems like it might be much simpler just to move the remaining handful of lines to better places (jl_has_meta could go in codegen.cpp, since it's only needed there, and I think jl_copy_ast would normally live in rtutils.c now).

The second commits seems to combine several semi-unrelated things that all happened to be in ast.c previously, into one struct, for no apparent reason. I get that there's a plan to replace parsing, but I don't think we also plan to, or need to, make flisp initialization (init) and print (the Informational queries) pluggable via the same interface.

@Keno
Copy link
Member

Keno commented May 18, 2019

The point is not to have a perfect interface, but to do the minimal thing that separates out all the flisp parts into a separate object that can be replaced wholesale. The informational queries depend on constants defined by the parser so they of course need to be replaced along with the parsing proper. @c42f wants to work on replacing lowering also, so we may as well move all the flisp-dependent code behind this interface. We can of course make it a simpler or more streamlined interface in future refactorings.

@c42f
Copy link
Member Author

c42f commented May 18, 2019

Yes, the idea here is to get a first version in to make it easier to iterate on things (I realize the interface leaves some things to be desired)

Having said that, code movement is painful and disruptive to other PRs so it would be nice to do a good job of it. I'll look at ast.c in a bit more detail and see whether the remaining stuff can be moved elsewhere.

@c42f
Copy link
Member Author

c42f commented Jul 27, 2019

I don't think we're going to do this in quite this way. Will reopen a new version if I get back to this later.

@c42f c42f closed this Jul 27, 2019
@DilumAluthge DilumAluthge deleted the cjf/frontend-api branch March 25, 2021 21:58
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