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

add strbasics.strip #16280

Merged
merged 26 commits into from
Feb 24, 2021
Merged

add strbasics.strip #16280

merged 26 commits into from
Feb 24, 2021

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 7, 2020

Ref RFC timotheecour#422

  • more generally, a lot of procs should become inline, the question is whether it'd be in some new strutils2 or in existing strutils (EDIT: some new module was the conclusion)

future work

@Araq
Copy link
Member

Araq commented Dec 7, 2020

I don't like the name too much and I dislike further strutils bloat. I do really like the code itself though. Not sure where we should put it though.

lib/pure/strutils.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member

timotheecour commented Dec 7, 2020

I don't like the name too much and I dislike further strutils bloat. I do really like the code itself though. Not sure where we should put it though.

then it's finally time for std/stringutils (we can bikeshed the name) with following properties:

  • low-dependency module (no dependencies on math, etc unlike strutils; possibly no dependency on unicode either)
  • efficiency first (only inplace procs, etc), defines building blocks used by other higher level modules; in particular focuses on inplace procs (and relies on sugar.dup + outplace argument _ RFCs#292)
  • std/strutils imports it and re-exports some symbols (more and more over time)
  • fixes some issues like the fact that strutils.isLowerAscii+friends can't be {.inline.} since it's rtl, as @xflywind noted in chat
  • moves some/all of compiler/strutils2.nim into std/stringutils
  • no {.rtl.} procs

note

  • can't be fusion/stringutils because the intent is to be reused by other stdlib modules
  • if needed, for a staging period we can put it in lib/std/private/stringutils.nim and move it to std/stringutils after some time

@ringabout ringabout marked this pull request as draft December 8, 2020 06:05
lib/std/stropt.nim Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
lib/pure/strutils.nim Outdated Show resolved Hide resolved
@ringabout ringabout marked this pull request as ready for review December 20, 2020 05:52
@ringabout ringabout changed the title add stripInplace to strutils add stropt.strip Dec 20, 2020
lib/std/stropt.nim Outdated Show resolved Hide resolved
lib/std/stropt.nim Outdated Show resolved Hide resolved
@ringabout ringabout marked this pull request as draft February 23, 2021 10:42
@ringabout ringabout marked this pull request as ready for review February 23, 2021 10:54
@konsumlamm
Copy link
Contributor

I still don't see why we need to make a new module for this. Why not put it in strmisc for example (as @juancarlospaco suggested)?

Also, it's not obvious at all what stropt is supposed to mean. "string optimized"? "string optional"? In particular, there are no module level doc comments explaining what this module is about.

@ringabout
Copy link
Member Author

ringabout commented Feb 23, 2021

I still don't see why we need to make a new module for this. Why not put it in strmisc for example (as @juancarlospaco suggested)?

  • strmisc depends on std/strutils which is a huge dependency for stropt which is intended for low level high performance string operations. And in fact std/strutils should imports this module and reexport some procs or variables.

  • stuffs which are in strmisc are for something uncommonly used while stropt should be used throughout Nim repo.

This module contains various string utility routines that are uncommonly used in comparison to strutils <strutils.html>_.

see also #16280 (comment)

then it's finally time for std/stringutils (we can bikeshed the name) with following properties:
low-dependency module (no dependencies on math, etc unlike strutils; possibly no dependency on unicode either)
efficiency first (only inplace procs, etc), defines building blocks used by other higher level modules; in particular focuses on inplace procs (and relies on sugar.dup + nim-lang/RFCs#292)
std/strutils imports it and re-exports some symbols (more and more over time)
fixes some issues like the fact that strutils.isLowerAscii+friends can't be {.inline.} since it's rtl, as @xflywind noted in chat
moves some/all of compiler/strutils2.nim into std/stringutils
no {.rtl.} procs

In the future PR, lots of things should be moved into std/stropt especially the functions in std/strutils can't be marked as inline which causes a lot of performance loss.

more generally, a lot of procs should become inline, the question is whether it'd be in some new strutils2 or in existing strutils

Also, it's not obvious at all what stropt is supposed to mean. "string optimized"? "string optional"? In particular, there are no module level doc comments explaining what this module is about.

It is called string optimized. We already have one line comment.

This module provides some high performance string operations.

@Araq
Copy link
Member

Araq commented Feb 23, 2021

Name it strbasics please.

@juancarlospaco
Copy link
Collaborator

What about...

Splitting a tiny in-place efficient charutils out of strutils,
charutils does not depend on strutils,
strutils imports charutils,
charutils has all proc that work on char, var char and collection of char,
strutils has all proc that work on string and var string, and collection of string,
makes strutils more lightweight because is kinda big,
charutils can be improved without touching strutils nor creating yet another strutils.

@ringabout ringabout changed the title add stropt.strip add strbasics.strip Feb 24, 2021
@Araq
Copy link
Member

Araq commented Feb 24, 2021

Splitting a tiny in-place efficient charutils out of strutils,

That's the plan, yes, but the focus on char isn't right, it's still about strings.

@Araq Araq merged commit 3f38f8f into nim-lang:devel Feb 24, 2021
doAssertRaises(AssertionDefect):
discard a.dup(setSlice(1 .. 11))

static: teststrip()
Copy link
Member

Choose a reason for hiding this comment

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

s/teststrip/main (since it's testing other procs

@@ -76,6 +76,8 @@
- Added `euclDiv` and `euclMod` to `math`.
- Added `httpcore.is1xx` and missing HTTP codes.

- Added `strip` and `setSlice` to `std/strbasics`.
Copy link
Member

Choose a reason for hiding this comment

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

  • Added std/strbasics containing strip and setSlice.

@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Feb 24, 2021
else:
# not JS and not Nimscript
when not declared(moveMem):
impl()
Copy link
Member

Choose a reason for hiding this comment

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

see whether we can use copyWithin see developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/copyWithin

(refs #16280 (comment))

ringabout added a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
@ringabout ringabout mentioned this pull request Mar 24, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@metagn metagn removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants