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

ir: Sanitize base names more aggressively #906

Merged
merged 2 commits into from
Aug 13, 2017

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Aug 11, 2017

This fixes stylo build failures when template instantiations tests caused a rust
compile error due to a bad test name.

This commit makes names better sanitized, preventing similar errors in the
future.

@emilio
Copy link
Contributor Author

emilio commented Aug 11, 2017

r? @Manishearth

cc @upsuper

src/ir/ty.rs Outdated
return Cow::Borrowed(name)
}

Cow::Owned(name.replace(|c| c == ' ' || c == ':' || c == '.' , "_"))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe assert that it is a valid identifier after this treatment?

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps just replace all non-ident chars?

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

r=me with fitzgen's assert

@emilio
Copy link
Contributor Author

emilio commented Aug 11, 2017

@bors-servo r=fitzgen,Manishearth

@bors-servo
Copy link

📌 Commit 20f2ed2 has been approved by fitzgen,Manishearth

@bors-servo
Copy link

⌛ Testing commit 20f2ed2 with merge fa89978...

bors-servo pushed a commit that referenced this pull request Aug 11, 2017
ir: Sanitize base names more aggressively

This fixes stylo build failures when template instantiations tests caused a rust
compile error due to a bad test name.

This commit makes names better sanitized, preventing similar errors in the
future.
@bors-servo
Copy link

💔 Test failed - status-travis

@emilio
Copy link
Contributor Author

emilio commented Aug 13, 2017

@bors-servo retry

  • Seems like a spurious failure?

@bors-servo
Copy link

⌛ Testing commit 20f2ed2 with merge ad3ddb7...

bors-servo pushed a commit that referenced this pull request Aug 13, 2017
ir: Sanitize base names more aggressively

This fixes stylo build failures when template instantiations tests caused a rust
compile error due to a bad test name.

This commit makes names better sanitized, preventing similar errors in the
future.
@bors-servo
Copy link

💔 Test failed - status-travis

emilio added 2 commits August 13, 2017 21:18
This fixes stylo build failures when template instantiations tests caused a rust
compile error due to a bad test name.

This commit makes names better sanitized, preventing similar errors in the
future.
@emilio
Copy link
Contributor Author

emilio commented Aug 13, 2017

or maybe I'm just stupid.

The assertion doesn't hold for every type because of stuff like struct anonymous (/path/to/file.c:line).

@bors-servo r=manishearth,fitzgen

@bors-servo
Copy link

📌 Commit b2bed30 has been approved by manishearth,fitzgen

@bors-servo
Copy link

⌛ Testing commit b2bed30 with merge 2996972...

bors-servo pushed a commit that referenced this pull request Aug 13, 2017
ir: Sanitize base names more aggressively

This fixes stylo build failures when template instantiations tests caused a rust
compile error due to a bad test name.

This commit makes names better sanitized, preventing similar errors in the
future.
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: manishearth,fitzgen
Pushing 2996972 to master...

@bors-servo bors-servo merged commit b2bed30 into rust-lang:master Aug 13, 2017
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