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

remove ties to stringification of io.write #13277

Closed
wants to merge 4 commits into from

Conversation

krux02
Copy link
Contributor

@krux02 krux02 commented Jan 28, 2020

Normally I would do such an implementation with macros, but they are not available in system.nim.

This is a breaking change. uses of write that rely on stringification won't work anymore. They will get an easy to fix compilation error though.

This PR is still raw. harmfulOverloadOfWrite isn't a good name, there is no test added, writeLn also needs to be adapted. I am just curious if a change like this could be welcome, or if I have to fight windmills again.

Personally a change like this would be really valuable to me. As you probably know I really don't link the $ stringification with its intermediate objects, and I don't like varargs. echo has both of it. I tried to get rid of the $ fallback in string interpolation, but I got massive negative feedback and it is back in there. write that works on File also has the $ fallback. There is simply nothing currently in Nim that doesn't secretly inject the $ function for printing in Nim. String interpolation and echo seems to popular to break, so I won't touch them, But write and writeLn could be non-popular enough to be used as a $ free printing environment.

would fix #13182

Very much like nim-lang/RFCs#191 this solution is extendable, but the extendable proc isn't a binary $ or an appendStr proc, it is just write, which we already have. But unlike that RFC write doesn't need a single intermediate string allocation, where the RFC does create one intermediate string object per call to echo or variadic write.

A generic $ could be implemented using an in memory file object, e.g. with FILE * open_memstream (char **ptr, size_t *sizeloc). But that is not very portable for all the backends.

@timotheecour
Copy link
Member

timotheecour commented Jan 29, 2020

I really don't link the $ stringification with its intermediate objects

maybe we need to replace existing $ overloads from system/dollars.nim by a bunch of appendStr + a single $ proc

# overloads like these
proc appendStr*(result: var string, a: int) = ...
proc appendStr*[T](result: var string, a: T) = ...
proc `$`*[T](a: T): string = appendStr(result, a)

(see also https://dlang.org/changelog/2.079.0.html#toString)

@krux02 krux02 changed the title remove ties to stringification of io.write, fixes #13182 remove ties to stringification of io.write, would fix #13182 Jan 29, 2020
@krux02
Copy link
Contributor Author

krux02 commented Jan 29, 2020

@timotheecour Yea, that would fix the problem. For quite a while I was hoping that add could take over that role. But then when I wasn't paying attention this happened without my approval: bab5e30

@timotheecour
Copy link
Member

timotheecour commented Jan 29, 2020

bab5e30 was a good change; result.add x was a source of bugs for x: int (or x: float) especially in generic code (user may expect it to add an int to an int etc); all we need is a better name than add; appendStr doesn't already exist and is not too long to type, so I suggest that name. Alternatives:

  • appendStr (doesn't exist)
  • addStr (doesn't exist, shorter, but a tiny bit less self-explanatory)

@krux02
Copy link
Contributor Author

krux02 commented Feb 5, 2020

@timotheecour I never had problems in my generic code trough add and I also never encountered issues here that were caused by that name. But ok I am also not that much attached to the name add. It is just that the names addFloat and addInt make it impossible to use in generic code unless you explicitly use when x is SomeFloat: or something like that.

@krux02 krux02 changed the title remove ties to stringification of io.write, would fix #13182 remove ties to stringification of io.write Feb 6, 2020
@timotheecour
Copy link
Member

timotheecour commented Feb 7, 2020

see #13345 for a cleaner fix IMO, which fixes the root cause of #13182 ; the other points (regarding temporaries and ties to stringification) remain valid, but hopefully there's a cleaner way to tackle that rather than introducing all those overloads (which cause generic bloat, for each unique instantiation of params, unlike varargs[string, $])

@krux02
Copy link
Contributor Author

krux02 commented Feb 8, 2020

@timotheecour First of all, yes you fixed a bug, and that is generally a better solution than breaking changes. However, I disagree that you fixed the root cause instead of a symptom. The root cause is the varargs overload that uses generic $. And thanks to problems like #11225 it can still happen that the local overload won't be taken and the generic varargs overload will be taken instead. There is also no way to opt out of the varargs[string, $] overload. So you can still never be sure that the program does what you expect it to do.

@timotheecour
Copy link
Member

timotheecour commented Feb 9, 2020

  • varargs[string,$] can now be overloaded after my fix but you can also always opt out of a proc with except, eg:
import system except write
system.write(stdout, 1234) # now needs FQ
stdout.write 0.1, " to_stdout ", 12, "\n" # prints 0.1 to_stdout 12

to have it available without dependency on macros.nim is also possible modulo a simple compiler patch (IIRC), as noted in RFC.

@krux02
Copy link
Contributor Author

krux02 commented Feb 10, 2020

@timotheecour, sometimes I really don't get you. How does import system except write me to opt out of the single overload of proc write(varargs[string, $])? It does opt out of all procs that have the name write.

@timotheecour
Copy link
Member

timotheecour commented Feb 10, 2020

import system except write
template write(f, a) = system.write(f, a)

then the example in #13182 works, effectively masking proc write*(f: File, a: varargs[string, $])

@krux02
Copy link
Contributor Author

krux02 commented Feb 10, 2020

@timotheecour ok I think I get the idea. But I would require typed templates and add templates for all types that I want to have. Not nice code, but it could work. Thank you.

@Araq
Copy link
Member

Araq commented Feb 26, 2020

Where is the RFC that covers this change? Where is the vote on the RFC? Where is the migration period?

@krux02
Copy link
Contributor Author

krux02 commented Feb 26, 2020

@Araq Well this is the RFC, but as a PR. I did it this way because of CI. The official RFCs repository does not have CI. The question though is, what should I do? Should I open an issue on the RFCs as well, to be formally correct? Should I elaborate there more about the type of problem that I am trying to fix? Or should I elaborate here more on it? If I should elaborate more, on what should I elaborate? What is the part that you don't understand?

Regarding the votes. Since when do we have a democracy in Nim?
Regarding the migration period. Migration is something that I would solve after the change has been accepted. I have little interest to invest a lot of time in something that you don't want to have in the first place, and then eventually get ignored and all my effort is wasted.

@Araq
Copy link
Member

Araq commented Feb 27, 2020

I have little interest to invest a lot of time in something that you don't want to have in the first place, and then eventually get ignored and all my effort is wasted.

Well exactly, so you start by writing an RFC that explains the benefits of your changed IO procs and then see if it can get a consensus. And then once accepted you can implement it, or you refer to an "already existing implementation" in this very PR, pointing out that all the tests are green, so that we know the code breakage at least doesn't affect the "important packages".

Regarding the votes. Since when do we have a democracy in Nim?

We don't, yet we value the feedback from the community and I don't use my dictatorship status all the time.

@krux02
Copy link
Contributor Author

krux02 commented Feb 27, 2020

Well exactly, so you start by writing an RFC that explains the benefits of your changed IO procs and then see if it can get a consensus. And then once accepted you can implement it, or you refer to an "already existing implementation" in this very PR, pointing out that all the tests are green, so that we know the code breakage at least doesn't affect the "important packages".

Well then I will an RFC pointing to the PR. For this change it was important to know ahead of time that no important packages would be broken by this change.

@stale
Copy link

stale bot commented Feb 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Feb 26, 2021
@stale stale bot closed this Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

write requires conversion to string
3 participants