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

Added cmpMem export #16484

Merged
merged 5 commits into from
Dec 29, 2020
Merged

Added cmpMem export #16484

merged 5 commits into from
Dec 29, 2020

Conversation

planetis-m
Copy link
Contributor

Currently there are zeroMem, copyMem, moveMem, equalMem but nimCmpMem is missing an export name.

lib/system/memalloc.nim Outdated Show resolved Hide resolved
@ringabout
Copy link
Member

ringabout commented Dec 27, 2020

general style guide

see timotheecour#415

  • code block => runnableExamples
  • rst code blocks without a test => recommend using a test (eg :test: "nim c $1"; perhaps with :status: 1)
  • proc + noSideEffect => func
  • assert in a test file => doAssert
  • isMainModule in stdlib => recommend moving to tests/stdlib/tfoo.nim
  • double backticks => single backtick
  • capitalize the first letter
  • lots of testament specific checks (eg exitcode: 0 usually useless)
  • etc, countless things that would be best done as a tool instead of relying on every reviewer to check all of those in every PR (plus for making it easier to improve stdlib + third party projects)
  • check for presence of multiple yield statements in inline iterators, which cause code bloat
  • see also: https://nim-lang.github.io/Nim/contributing.html#best-practices
  • naming style https://nim-lang.github.io/Nim/nep1.html

@timotheecour
Copy link
Member

instead of exporting a new symbol in system, how about creating a new low-level stdlib module std/memory (or maybe std/memutils) which can then grow as needed without being burdened by bloating system.nim?

(note, there's already system/memory.nim but it's considered "private")

As usual, code duplication can be avoided by using intermediate private modules, eg import std/private/x)

## Compares the memory blocks ``a`` and ``b``. ``size`` bytes will
## be compared.
##
## Returns:
Copy link
Member

@timotheecour timotheecour Dec 28, 2020

Choose a reason for hiding this comment

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

needs a runnableExamples + test in tests/stdlib/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

runnableExamples doesn't work, I'm getting line 49: invalid identation

Copy link
Member

Choose a reason for hiding this comment

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

@planetis-m that's because you need = in proc declaration; followup PR welcome

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would refactor the system module, I guess there is a good reason it was written that way, if you need runnableexamples so bad you can try.

Copy link
Member

Choose a reason for hiding this comment

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

indeed, see nim-lang/RFCs#309

@Araq
Copy link
Member

Araq commented Dec 28, 2020

instead of exporting a new symbol in system, how about creating a new low-level stdlib module std/memory (or maybe std/memutils) which can then grow as needed without being burdened by bloating system.nim?

Yeah, I agree. However, this should be done later, for now it's conistent with copyMem and friends.

@planetis-m
Copy link
Contributor Author

Thanks for the review everyone, I will address all your comments today (so don't merge yet).

@planetis-m
Copy link
Contributor Author

instead of exporting a new symbol in system, how about creating a new low-level stdlib module std/memory (or maybe std/memutils) which can then grow as needed without being burdened by bloating system.nim?

(note, there's already system/memory.nim but it's considered "private")

As usual, code duplication can be avoided by using intermediate private modules, eg import std/private/x)

Can't work, I tried it and getting ambiguous calls. It would be a breaking change.

@Clyybber Clyybber merged commit d5a3c2c into nim-lang:devel Dec 29, 2020
@planetis-m planetis-m deleted the cmpmem branch December 29, 2020 12:36
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* added cmpMem export

* updates

* fix test

* Tiny changelog change

* Add a dot.

Co-authored-by: Clyybber <darkmine956@gmail.com>
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* added cmpMem export

* updates

* fix test

* Tiny changelog change

* Add a dot.

Co-authored-by: Clyybber <darkmine956@gmail.com>
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.

5 participants