Skip to content

Conversation

@greenhat
Copy link
Contributor

@greenhat greenhat commented Jun 24, 2024

This PR is stacked on the #218 and should be merged after it

Use FileName::as_str for source name in Wasm translation to avoid <name> (enclosed in brackets) which is produced by FileName::to_string since the brackets are illegal in MASM module ids.

@bitwalker
Copy link
Collaborator

bitwalker commented Jun 24, 2024

Having thought about this some more, I think actually we should tackle this issue a bit differently. See my comment here. I'm going to leave this marked approved for now, so we can merge it as a temporary workaround, but the fundamental issue here requires a more appropriate fix I think. We can address it properly in another PR, or in this one, whichever you think is more appropriate.

@greenhat greenhat force-pushed the greenhat/i213-env-var-skip-rust-comp branch from cbb7e91 to 3ec7ed7 Compare June 25, 2024 11:13
@greenhat greenhat force-pushed the greenhat/fix-virtual-filename-brackets branch from 91adfc2 to e511221 Compare June 25, 2024 11:14
@greenhat
Copy link
Contributor Author

Having thought about this some more, I think actually we should tackle this issue a bit differently. See my comment here. I'm going to leave this marked approved for now, so we can merge it as a temporary workaround, but the fundamental issue here requires a more appropriate fix I think. We can address it properly in another PR, or in this one, whichever you think is more appropriate.

I'm not sure if I follow. I'm using "unbracketed" name here to avoid ConvertHirToMasm failing at https://github.com/0xPolygonMiden/compiler/blob/8b8117fd95c04b3e3f8b4016754bf0c1fdbad1d6/codegen/masm/src/convert.rs#L117
I suppose we can leave brackets but treat every module name there as raw ids. Is that the idea?

@bitwalker
Copy link
Collaborator

Right, my bad, I got this one mixed in with other issues related to module/procedure paths

@greenhat greenhat force-pushed the greenhat/fix-virtual-filename-brackets branch from e511221 to 9d08a68 Compare June 26, 2024 06:03
@greenhat greenhat force-pushed the greenhat/i213-env-var-skip-rust-comp branch from 3ec7ed7 to bf08ddb Compare June 26, 2024 06:03
Base automatically changed from greenhat/i213-env-var-skip-rust-comp to main June 26, 2024 07:48
@greenhat greenhat merged commit 2a1e66d into main Jun 26, 2024
@greenhat greenhat deleted the greenhat/fix-virtual-filename-brackets branch June 26, 2024 07:48
greenhat added a commit that referenced this pull request Jul 2, 2024
Which was added as a workaround in #220 for the issue #208.
greenhat added a commit that referenced this pull request Jul 11, 2024
Which was added as a workaround in #220 for the issue #208.
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