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

[WIP] Source map #36

Merged
merged 20 commits into from
Oct 15, 2018
Merged

[WIP] Source map #36

merged 20 commits into from
Oct 15, 2018

Conversation

FloorLamp
Copy link
Contributor

No description provided.

@@ -39,6 +39,8 @@ let argspec = Arg.align
"--dfinity",
Arg.Unit (fun () -> compile_mode := Pipeline.DfinityMode),
" compile for dfinity";
"--map", Arg.Set Flags.source_map, " output source map";
"--disable-prelude", Arg.Clear Flags.prelude, " disable prelude";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to be able to disable the prelude?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More of a temporary thing to reduce the output size, and to avoid dealing with prelude as a source

src/encodeMap.ml Outdated Show resolved Hide resolved
src/encodeMap.ml Outdated
Vlq.Base64.encode buf4 (ic - !prev_ic); (* input column *)
map := !map @ [Buffer.contents buf1; Buffer.contents buf2; Buffer.contents buf3; Buffer.contents buf4] @ [","];

if !filename = "" then filename := file;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That looks hackish. Shouldn’t there be a list of source files, and the sources index point to the corresponding one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, fixed now

Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

This is going into the right direction, thanks a lot!

Eventually we want to default to foo.wasm if the input is foo.as (and probably only if there is only one) - you can add that, if you want, but don't feel obliged; I can do that when I get back to coding. (It can be discouraging if a contribution gets tired to cleaning up other people's code :-))

@FloorLamp
Copy link
Contributor Author

Added!

Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Great! :-)

@FloorLamp
Copy link
Contributor Author

Ok, this can probably be merged now, @nomeata?

src/js_main.ml Outdated
method compileWat mode s = js_compile_wat mode s
method compileWasm mode s = js_compile_wasm mode s
method compileWat mode s source_map = js_compile_wat mode s source_map
method compileWasm mode s source_map = js_compile_wasm mode s source_map
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep the source string as the last argument?

Copy link
Contributor Author

@FloorLamp FloorLamp Oct 10, 2018

Choose a reason for hiding this comment

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

I actually prefer source string as the first arg, so we can do this:

asc.compileWasm(source, { mode: 'dfinity', sourceMap: true, ... })

Copy link
Contributor

Choose a reason for hiding this comment

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

The motivation behind my asking is that making the input the last parameter generally supports function composition and partial application.

I think this describes it quite well: https://wiki.haskell.org/Parameter_order

src/encodeMap.ml Show resolved Hide resolved
src/encodeMap.ml Show resolved Hide resolved
@FloorLamp
Copy link
Contributor Author

Refactored a bit - moved SourceMap into its own module but there is still code duplication

@nomeata
Copy link
Collaborator

nomeata commented Oct 11, 2018

Refactored a bit - moved SourceMap into its own module but there is still code duplication

But now we have a code copy that only produce a SourceMap (but not the wasm), and no longer produces an unusd wasm module, right? But this means that the SouceMap will be out-of-sync if we upgrade wasm and the encoding changes…

Wouldn’t it be cleaner to just use the wasm from the copied encoding module (and not use the one from wasm at all)? Sorry when I was less clear about this earlier.

@FloorLamp
Copy link
Contributor Author

That was my initial idea, I can revert if we want to do that.

@nomeata
Copy link
Collaborator

nomeata commented Oct 11, 2018

That was my initial idea, I can revert if we want to do that.

I am in favor. Seems the lesser of two evils.

@FloorLamp
Copy link
Contributor Author

Ok, I reverted back to encodeMap. Also moved source to the last arg in the exported js methods

@rossberg
Copy link
Contributor

Okay, I don't have a better suggestion for now, so SGTM.

@nomeata nomeata merged commit 68aa5f1 into master Oct 15, 2018
@nomeata
Copy link
Collaborator

nomeata commented Oct 15, 2018

Merged!

@nomeata nomeata deleted the source-map branch October 15, 2018 14:51
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.

4 participants