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

Change symbol mangling on Windows to avoid conflicts #5518

Merged
merged 1 commit into from Jan 3, 2018

Conversation

Vexatos
Copy link
Contributor

@Vexatos Vexatos commented Jan 3, 2018

Previously, certain symbols produced conflicts on windows, since ** was mangled into .. which was the same as what + was mangled into, producing undefined behaviour (In this case, it seems that + was being executed instead of **). This change mangles non-alphanumerical characters into codes with their (upper-case) hex values (e.g. < is mangled into .3C.), which will avoid conflicts and, in case of issues, still show which symbol used to be there, at the cost of being rather verbose.

@RX14 RX14 requested a review from asterite January 3, 2018 15:16
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

I think it might be better to use String.build here, because otherwise this will allocate a lot of memory in the codegen phase. Previously it didn't because there was no memory allocation in the block.

@asterite
Copy link
Member

asterite commented Jan 3, 2018

I made a comment, but feel free to merge this. We can improve the performance later, specially because now there's no windows support at all.

@Vexatos
Copy link
Contributor Author

Vexatos commented Jan 3, 2018

Changed the fix according to the suggestion to use String.build instead of string interpolation.

@asterite
Copy link
Member

asterite commented Jan 3, 2018

I guess that could happen... ¯_(ツ)_/¯

@Vexatos I meant using String.build instead of string.gsub. Something like:

String.build do |io|
  name.each_char do |char|
    case char
      ...
    end
  end
end

@Vexatos
Copy link
Contributor Author

Vexatos commented Jan 3, 2018

Ah, yeah, sorry, that would probably be a lot faster than gsub.

@Vexatos
Copy link
Contributor Author

Vexatos commented Jan 3, 2018

Also not mangling _ now since that is a fairly common character that does not need to be mangled.

@ysbaddaden
Copy link
Contributor

Do we need to mangle specifically for Windows anymore?

@asterite
Copy link
Member

asterite commented Jan 3, 2018

I just tried without the special mangling for Windows and the object file ends up being invalid (when executing the cl command on Windows)

@RX14 RX14 merged commit 44a4c63 into crystal-lang:master Jan 3, 2018
lukeasrodgers pushed a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018
@straight-shoota
Copy link
Member

Can we get this cherry-picked for 0.24.2? Would be a great help for the ongoing windows efforts. @bcardiff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants