-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Don't leak variable name when assigning to ivar/cvar in method signature #6007
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -312,9 +312,10 @@ struct Range(B, E) | |
|
|
||
| @range : Range(B, E) | ||
| @current : B | ||
| @reached_end : Bool | ||
| @reached_end = false | ||
|
|
||
| def initialize(@range : Range(B, E), @current = range.begin, @reached_end = false) | ||
| def initialize(@range : Range(B, E)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here you're also changing the ctor signature, we can no more setup current & reached_end. Is that intentional?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a private class. And the removed arguments don't seem to be used anywhere. Wouldn't make much sense anyway.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The constructor was used internally, so no problem. Same in other places. |
||
| @current = @range.begin | ||
| end | ||
|
|
||
| def next | ||
|
|
@@ -348,7 +349,8 @@ struct Range(B, E) | |
| @range : Range(B, E) | ||
| @current : E | ||
|
|
||
| def initialize(@range : Range(B, E), @current = range.end) | ||
| def initialize(@range : Range(B, E)) | ||
| @current = @range.end | ||
| rewind | ||
| end | ||
|
|
||
|
|
@@ -374,9 +376,10 @@ struct Range(B, E) | |
| @range : R | ||
| @step : N | ||
| @current : B | ||
| @reached_end : Bool | ||
| @reached_end = false | ||
|
|
||
| def initialize(@range, @step, @current = range.begin, @reached_end = false) | ||
| def initialize(@range, @step) | ||
| @current = @range.begin | ||
| end | ||
|
|
||
| def next | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,7 +50,8 @@ class Socket | |
| @addr6 : LibC::In6Addr? | ||
| @addr4 : LibC::InAddr? | ||
|
|
||
| def initialize(@address : String, @port : Int32) | ||
| def initialize(address : String, @port : Int32) | ||
| @address = address | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? Ditto a few times below, where inline ivar init is ok.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wow, that's weird to read ^^ but makes sense, ty |
||
| if @addr6 = ip6?(address) | ||
| @family = Family::INET6 | ||
| @size = sizeof(LibC::SockaddrIn6) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can/Should this check if the name already exists in the current scope and discard it? Say I do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is that variables that start with
__are reserved for the compiler, so they shouldn't clash with user variables. If a user choose to name their variables__argN, they are warned that that's problematic (at least in my mind :-P).I don't want to make things more complex than they should, at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh, not asking to do this here, but do you think it would actually be possible to make trying to use these a parser error then? I don't think it's too uncommon for somebody to try to use such variable names, especially when dealing with macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it can be done. Sometimes methods are passed to macros, and these are
to_s'd, and then reparsed again... and that would error when it shouldn't.Maybe it's something to consider in the future.
Note that right now the
__argor__temphack is used all over the place in the compiler (like, when you doif a() || b(), this isn't something new. And so far it didn't cause any problem, so I guess it's fine.