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

[11/x] UseFileName::as_strfor source file name to avoid enclosing virtual filenames in brackets #220

Merged
merged 1 commit into from
Jun 26, 2024

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
Contributor

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

panic!("invalid module name '{}': {}", module.name.as_str(), err)

I suppose we can leave brackets but treat every module name there as raw ids. Is that the idea?

@bitwalker
Copy link
Contributor

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
8 checks passed
@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.

2 participants