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.

Sign up for GitHub

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

Jump to bottom

Deprecate TaintedString #15423

Merged
merged 33 commits into from
Jan 16, 2021
Merged

Deprecate TaintedString #15423

merged 33 commits into from
Jan 16, 2021

Conversation

juancarlospaco
Copy link
Collaborator

@juancarlospaco juancarlospaco commented Sep 28, 2020 •
edited
Loading

@Araq
Copy link
Member

Araq commented Sep 28, 2020 •
edited
Loading

I like this PR but is this covered by an RFC? Anybody out there who likes TaintedStrings?

@Vindaar
Copy link
Contributor

Vindaar commented Sep 28, 2020

Anybody out there who likes TaintedStrings?

To me TaintedString was one of the first instances of discovering how much stricter and advanced Nim's type system is over C's. Was a powerful moment in a way.

But have I ever used it myself? Nah, for my kind of coding I never felt it'd give me any benefits.

So personally, I wouldn't miss it. I always felt like it could be a neat feature for the right kind of software though.

@juancarlospaco juancarlospaco marked this pull request as ready for review September 28, 2020 21:08
@juancarlospaco juancarlospaco changed the title Clean out WIP Deprecate TaintedString Sep 28, 2020
@mratsim
Copy link
Collaborator

mratsim commented Oct 6, 2020

I like TaintedString but it needs more fleshed out support.

@juancarlospaco
Copy link
Collaborator Author

@Araq This is ready for Merge. 🙂

@disruptek
Copy link
Contributor

I like TaintedString as a concept but I think it'd be much easier to tolerate if it was implemented in the frontend as tyAlias or something so that it isn't virtually indistinguishable from tyString. This could be my naivety speaking, though.

@Araq
Copy link
Member

Araq commented Oct 14, 2020

I really think this should go through the RFC process.

@juancarlospaco
Copy link
Collaborator Author

ReSync with devel again.

@Araq
Copy link
Member

Araq commented Oct 30, 2020

RFC, where it is?

@juancarlospaco juancarlospaco mentioned this pull request Nov 20, 2020
Closed
@FedericoCeratto
Copy link
Member

Is this the related RFC? nim-lang/RFCs#24

@juancarlospaco juancarlospaco mentioned this pull request Jan 4, 2021
Closed
@juancarlospaco
Copy link
Collaborator Author

I dont know theres like ~3 issue/rfc/etc that talk about this. 🤷

timotheecour
timotheecour reviewed Jan 14, 2021
View reviewed changes
lib/system.nim Outdated Show resolved Hide resolved
timotheecour
timotheecour reviewed Jan 14, 2021
View reviewed changes
changelog.md Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Jan 15, 2021

Please address Timothee's comments and I'm happy to merge it, the RFC has finally been accepted.

juancarlospaco and others added 9 commits January 15, 2021 19:54
@juancarlospaco @dom96
Update lib/pure/asyncftpclient.nim
7df8653 
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
@juancarlospaco @dom96
Update lib/pure/asyncftpclient.nim
43eb6ea 
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
@juancarlospaco @dom96
Update lib/pure/asyncftpclient.nim
f304473 
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
@juancarlospaco @dom96
Update lib/impure/rdstdin.nim
9e28c19 
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
@juancarlospaco @dom96
Update lib/impure/rdstdin.nim
ccd0051 
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
@juancarlospaco @dom96
Update lib/impure/rdstdin.nim
a0de47e 
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
@juancarlospaco
ReSync
7eb8f09
@juancarlospaco @timotheecour
Update compiler/commands.nim
2a5cc45 
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
@juancarlospaco
ReSync
4ae6b49
@timotheecour timotheecour mentioned this pull request Jan 15, 2021
Closed
timotheecour
timotheecour reviewed Jan 15, 2021
View reviewed changes
compiler/commands.nim Outdated Show resolved Hide resolved
timotheecour
timotheecour reviewed Jan 15, 2021
View reviewed changes
compiler/commands.nim Outdated Show resolved Hide resolved
juancarlospaco and others added 2 commits January 15, 2021 20:25
@juancarlospaco @timotheecour
Update compiler/commands.nim
eb839c1 
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
@juancarlospaco @timotheecour
Update compiler/commands.nim
8e70d1e 
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
timotheecour
timotheecour approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

LGTM after last comment

@timotheecour timotheecour mentioned this pull request Jan 15, 2021
Closed
@timotheecour timotheecour added the merge_when_passes_CI mergeable once green label Jan 16, 2021
@timotheecour timotheecour merged commit 78a9958 into nim-lang:devel Jan 16, 2021
@timotheecour timotheecour deleted the no-taint branch January 16, 2021 02:56
@timotheecour timotheecour mentioned this pull request Jan 16, 2021
Closed
@timotheecour
Copy link
Member

timotheecour commented Jan 19, 2021 •
edited
Loading

  • @juancarlospaco good PR but IMO a followup is needed to remove all the hints that stem from it eg:
testament/testament.nim(803, 44) Hint: conversion from string to itself is pointless [ConvFromXtoItselfNotNeeded]

EDIT => #16764

This was referenced Jan 20, 2021
Merged
Merged
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@juancarlospaco @timotheecour
@dom96 @ardek66
Deprecate TaintedString (nim-lang#15423)
e4faaef 
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timotheecour timotheecour timotheecour approved these changes

@dom96 dom96 dom96 approved these changes

Assignees
No one assigned
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@juancarlospaco @Araq @Vindaar @mratsim @disruptek @FedericoCeratto @timotheecour @dom96