-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix subclasses of string. (Fixes #130) #674
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
Conversation
|
||
if args.args.len() > 2 { | ||
panic!("str expects exactly one parameter"); | ||
fn str_new( |
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.
Now that methods exactly match their python names, should we do the same for free-standing functions? i.e. should this just be str
?
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.
Maybe. I quite like the convention, it makes it a bit easier to know what's what. I think I'd call that new
rather than str
though.
Of course, that function could be a method.
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.
But it can't be a method called new
because that clashes with the implementation of new in PyRef.
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 really like the idea of it being a new
method though (feels more idiomatic). We can probably get rid of PyRef::new
and have a generic into_ref
on PyValue
s.
(doesn't need to be in this PR though, just tossing out ideas)
|
||
## Base strings currently get an attribute dict, but shouldn't. | ||
# with assertRaises(AttributeError): | ||
# "hello".x = 5 |
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 think it is more consistent that base strings have attribute dicts, I believe cpython is wrong here. We can deviate. You can use the platform module to check for rustpython.
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.
It feels like a pretty serious performance question, if we have to allocate dicts for intermediate ints and bools in computations.
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.
It's important enough that __slots__
exists so that user classes can take advantage of this.
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.
It feels like a pretty serious performance question, if we have to allocate dicts for intermediate ints and bools in computations.
Not just that; this also makes other common optimizations on builtin types that we would probably implement at some point (string interning, small int cache) much weirder and harder to reason about.
In CPython:
>>> a = "hello"
>>> b = "hello"
>>> a is b
True
>>> a = 123
>>> b = 123
>>> a is b
True
So if in one section of code I do "hello".x = 5
, this may affect any other code (including third-party library) using the same "hello" literal - as it may or may not be interned. Same with other builtins. That's implicit global state.
Codecov Report
@@ Coverage Diff @@
## master #674 +/- ##
==========================================
- Coverage 40.33% 40.19% -0.14%
==========================================
Files 79 79
Lines 17488 17456 -32
Branches 4470 4465 -5
==========================================
- Hits 7054 7017 -37
- Misses 8588 8607 +19
+ Partials 1846 1832 -14
Continue to review full report at Codecov.
|
586eb71
to
5b41cc4
Compare
The remaining discussions are beyond the scope of this PR, so I think this should be ok to merge? |
No description provided.