-
-
Notifications
You must be signed in to change notification settings - Fork 649
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
add #:immutable? keyword argument to functions that return strings, byte-strings, or vectors #1341
base: master
Are you sure you want to change the base?
Conversation
…functions Adds an optional #:immutable? keyword argument for operations that return strings, byte-strings, or vectors. If the #:immutable? argument is a truthy value, the string, byte-string, or vector it returns will be immutable.
I was analyzing the problem with the tests. The optimizer can't inline keywords procedures, so it can't see that A real fix is difficult. It's necessary to force the inlining of the internal procedure (Perhaps it's possible to fix these issues with some macros. Instead of creating a function My easy alternative is to modify the test meanwhile. I wrote a commit that changes Commit: gus-massa@5a9a467 ( travis ) (look near optimize.rktl#L1359 ) By the way, you forgot struct->vector . |
Oh, okay. Would it make sense to make it a non-keyword argument somehow? But then what about functions like |
I think it's better to keep your version with the keywords because it looks better for usability, but I don't know the opinion of the rest. I hope it's possible to fix the optimization details later. As a proof of concept, I wrote a version of
|
But this wouldn't work if I made the optional argument default to the value of a |
I totally missed the part about parameters. In my example, it can be fixed with something like
Now the reductions are not perfect, but IMHO, it's a bad idea to use parameters in a function like In this case, I'd like to have a fixed default. I'd like to make the immutable version the default, but it's backwards incompatible, so I think this change should be delayed to Racket2. So, I think that the next best option is to add the keyword but keep the mutable version as the default. This can make some calculations slower, but the difference can be fixed. Another comment about my horrible code: It has a few wrong corner cases, for example
|
Ok, so for lower-level functions like |
I found a small problem in your implementation with the message of the error when a function has an implicit argument, for example in:
The error is
Instead of
I was looking at the implementation of keyword procedures. kw.rkt#L1026 The implementation binds the identifier in the definition to a macro that expands to a hidden procedure. So I guess it's possible to change the implementation of all the keyword procedures to something like the macro that I wrote before (fixing all the details, corner cases and errors :)). But it's out of scope of your commit, perhaps I should send a feature request to the mailing list, to try to convince someone that knows the details to fix it. I'd like that in some code like
At least Extra points for expanding I made a few small tests, and apparently the problem is only with optional keywords, all the other cases seams to be already written in the more efficient way. :) |
No, I don't think this has anything to do with keyword arguments. It's because I'm doing the equivalent of this: (define (my-make-vector n [v 0])
(make-vector n v))
(my-make-vector -1) And that means that the core |
I agree that there are two different problems. One is that the optional keyword arguments are expanded in a way that is not friendy to the optimizer. This has to be fixed elsewhere. The other problem is that after using the automatic optional argument, the distintion between a missing argument and one that coincides dissapears, so in case of an error, the end user can get confused by the ghost argument. I was thinking in something like:
(my-make-vector -1) (It's not 100% optimizer friendly, but it raise the same errors that the |
This adds an optional
#:immutable?
keyword argument to all of the functions fromracket/base
(that I could think of) that currently return mutable strings, byte-strings, or vectors.This is an alternative to #1219.