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

String Interpolation #52

Merged
merged 21 commits into from Apr 14, 2017
Merged

String Interpolation #52

merged 21 commits into from Apr 14, 2017

Conversation

segiddins
Copy link
Contributor

This is broken and I don't know why ¯_(ツ)_/¯

case semicolon
case newline
case leftParen
case rightParen
case leftBrace
case slashLeftParen
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this can be removed now

@@ -130,12 +133,14 @@ enum TokenKind: Equatable {
case .identifier(let value): return value
case .unknown(let char): return char
case .char(let value): return String(UnicodeScalar(value))
case .stringInterpolationLiteral(_): return "literal w/ interp"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually implement this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cant implement this is-is, since the current data structure doesn't distinguish between interpolated & non-interpolated parts of the string literal

Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly don't think this is even called. I wouldn't worry about it.

@@ -544,14 +576,6 @@ struct Lexer {
advance(3)
return Token(kind: .ellipsis, range: range(start: startLoc))
}
if c == "[" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed since the c.isOperator case should handle them

@harlanhaskins
Copy link
Collaborator

This is some fantastic work, and I am SO excited to merge this.

stdlib/String.tr Outdated
} else if value is UInt16 {
fatalError("unimplemented")
} else if value is UInt32 {
fatalError("unimplemented")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the snprintf stuff sucks to repeat, but could you implement these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I can. Can't wait until we have generics and can implement it just once!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

stdlib/String.tr Outdated
fatalError("unimplemented")
} else if value is Bool {
if value as Bool {
self._storage = ByteArray("true" as *Int8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does self = "true" not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, does assigning to self actually work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does -- it's just a store into the alloca. I just tried it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -130,12 +133,14 @@ enum TokenKind: Equatable {
case .identifier(let value): return value
case .unknown(let char): return char
case .char(let value): return String(UnicodeScalar(value))
case .stringInterpolationLiteral(_): return "literal w/ interp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly don't think this is even called. I wouldn't worry about it.

@segiddins segiddins force-pushed the seg-interpolate-all-the-things branch from 208fd6a to 99811ba Compare March 27, 2017 20:57
@segiddins
Copy link
Contributor Author

99811ba is optional, it will have a major performance downside

@harlanhaskins
Copy link
Collaborator

It certainly will, but I think that's actually fine. Constructing a temporary string isn't the end of the world.

@segiddins
Copy link
Contributor Author

segiddins commented Mar 27, 2017

Failing due to the segfault @CodaFi wanted to look into

@CodaFi
Copy link
Contributor

CodaFi commented Mar 29, 2017

I think the crashes you're seeing here are actually caused by this patch. If it were LLVM you'd see it in the frames.

@segiddins
Copy link
Contributor Author

They're happening when disposing the module, I'm pretty sure

@CodaFi
Copy link
Contributor

CodaFi commented Mar 29, 2017

It would go through LLVMDisposeModule like the others if that were true.

@segiddins
Copy link
Contributor Author

It's being caught in the signal handler...

@harlanhaskins
Copy link
Collaborator

@trill-ci please test

@segiddins segiddins force-pushed the seg-interpolate-all-the-things branch from 305c4b8 to c3a5dfb Compare March 30, 2017 02:24
@segiddins
Copy link
Contributor Author

@trill-ci please pass

@harlanhaskins
Copy link
Collaborator

Uh...this should probably be reduced and filed on lira.

@segiddins
Copy link
Contributor Author

I need to do reading tonight and go to bed and tbh I have no idea where to start with reduction

@segiddins segiddins force-pushed the seg-interpolate-all-the-things branch from 62e9e6c to c94a446 Compare April 13, 2017 22:55
@harlanhaskins harlanhaskins merged commit ac07f64 into master Apr 14, 2017
@harlanhaskins
Copy link
Collaborator

🎉 🎉 🎉

@harlanhaskins harlanhaskins deleted the seg-interpolate-all-the-things branch April 14, 2017 01:47
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.

None yet

3 participants