Skip to content

Refactor jsoo rules to use Compilation_context.t#1048

Merged
rgrinberg merged 3 commits intoocaml:masterfrom
rgrinberg:jsoo-cctx
Jul 31, 2018
Merged

Refactor jsoo rules to use Compilation_context.t#1048
rgrinberg merged 3 commits intoocaml:masterfrom
rgrinberg:jsoo-cctx

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

This is how things are done in the menhir plugin for example. The jsoo rules are a bit older so haven't yet been refactored to use this type.

cc @hhugo

PS: Should we consider making the jsoo rules a proper plugin eventually?

@rgrinberg rgrinberg requested a review from a user July 24, 2018 11:31
@rgrinberg rgrinberg force-pushed the jsoo-cctx branch 2 times, most recently from 7b784a7 to 6f2b25f Compare July 24, 2018 13:06
end

let build_cm cctx =
let module Jsoo = Gen(struct let cctx = cctx end) in
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgrinberg it's not immediately clear to me what is the cost of doing this. For instance the Gen functor will now be applied for every .ml file. It's fine if it only contain functions, but if we start adding toplevel values in the future performances might degrade.

I think we should stick to passing cctx explicitly here. Adding module C = Compilation_context should be enough to remove most of the clutter.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but I think the menhir also does this. I'll just get rid of the functor, the boilerplate wasn't even that bad.

@ghost
Copy link
Copy Markdown

ghost commented Jul 30, 2018

PS: Should we consider making the jsoo rules a proper plugin eventually?

Maybe, also it's a complicated kind of plugin given that it needs to be applied to the whole workspace.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg merged commit a13325e into ocaml:master Jul 31, 2018
@rgrinberg rgrinberg deleted the jsoo-cctx branch July 31, 2018 07:36
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.

1 participant