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

misc cleanups #64

Open
1 task
timotheecour opened this issue Mar 12, 2020 · 51 comments
Open
1 task

misc cleanups #64

timotheecour opened this issue Mar 12, 2020 · 51 comments
Labels

Comments

@timotheecour
Copy link
Owner

timotheecour commented Mar 12, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Mar 14, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Mar 14, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Mar 17, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Mar 26, 2020

@timotheecour timotheecour changed the title cleanups misc cleanups Apr 4, 2020
@timotheecour
Copy link
Owner Author

timotheecour commented Apr 7, 2020

deprecate isNil

see also timotheecour/D_vs_nim#36 (comment)

Edited:
nil == something doesn't work with C or JS backend sometime.

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 9, 2020

  • importcpp > importjs for js

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 12, 2020

  • replace
## some ``foo``
=>
## some `foo`

everywhere in nim files since it's simpler and compat w markdown

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 16, 2020

  • cleanup $nim_prs_D/tests/vm/tvmops.nim
template forceConst(a: untyped): untyped =
  ## Force evaluation at CT, useful for example here:
  ## `callFoo(forceConst(getBar1()), getBar2())`
  ## instead of:
  ##  block:
  ##    const a = getBar1()
  ##    `callFoo(a, getBar2())`
  const ret = a
  ret

=> a.static

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 18, 2020

=> nim-lang#14732

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 19, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 21, 2020

  • proc paramStr*(i: int): TaintedString = raise newException(OSError, "paramStr is not implemented on Genode")
    => this should be a different kind of Exception, maybe doAssert false

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 27, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 29, 2020

  • remove all absolute html links and replace with relative links eg:
<https://nim-lang.org/docs/lib.html#pure-libraries-modules-for-js-backend>`_

@timotheecour
Copy link
Owner Author

timotheecour commented Apr 29, 2020

this is related to nim-lang#11783 but seems different, as the bug in nim-lang#11783 was only present when both appended and appendee were uninitialized, which isn't the case here.

var ret = "foo1"
ret.add fun()
)

@timotheecour
Copy link
Owner Author

timotheecour commented May 7, 2020

nim-lang#14066 (comment)

By the way, what is the tyOpt type that can be found next to seq in the code?
It is a builtin "opt[T]" type, it's dead code.

@timotheecour
Copy link
Owner Author

timotheecour commented May 7, 2020

  • turn all code-block (which aren't being compiled/run!) into runnableExamples now that most runnableExample issues have been fixed (including multiple runnableExamples at module scope).

eg of big offender:
lib/system.nim 87
lib/pure/strutils.nim 36
lib/pure/math.nim 31

total:

as of v1.4.0: 568
as of devel 52cf7280019c943dd7df33d0dd693931e6a116ee: 500

so it's going down...
(using rg code-block: -t nim | wc -l: 500)

/cc @konsumlamm thanks for your help on this with your cleanup PR's!

@timotheecour
Copy link
Owner Author

timotheecour commented May 13, 2020

  • cleanup these:
    let message = "$1($2, $3) $4: $5" % [filename, $line, $col, $mc, a] (rst.nim)
    to use toLocation

@timotheecour
Copy link
Owner Author

timotheecour commented May 25, 2020

  • tests should use separate files, not isMainModule, see why here

move test to tests/stdlib/tsequtils.nim like most modules; rationale:

nim-lang#14413 (comment)

@timotheecour
Copy link
Owner Author

timotheecour commented May 31, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Jun 4, 2020

  • best practice: give warning when an iterator has multiple yield statements

$nim_prs_D/doc/contributing.rst
eg: in nimbleSubs

mentioned in https://nim-lang.github.io/Nim/manual.html#iterators-and-the-for-statement-first-class-iterators but hard to see

Caution: the body of a for loop over an inline iterator is inlined into each yield statement appearing in the iterator code, so ideally the code should be refactored to contain a single yield when possible to avoid code bloat.

@timotheecour
Copy link
Owner Author

timotheecour commented Jun 7, 2020

deadcode

  • sfIsSelf
  • skConditional
  • remove entries marked deadcode

@timotheecour
Copy link
Owner Author

timotheecour commented Jun 10, 2020

  • compiler/options.nim removeTrailingDirSep
    =>
    let nimblePath = normalizePathEnd(conf.nimblePaths[i].string.byLent)

with:

##[
unstable API, internal use only for now.
candidate for merging in std/decls
]##

proc byLent*[T](a: var T): lent T =
  ## caveat: see https://github.com/nim-lang/Nim/issues/14557
  ## use case: convert a var to a lent to help with overload resolution, see
  ## https://github.com/timotheecour/Nim/issues/287
  a
  

@timotheecour
Copy link
Owner Author

factor vmconv.elementType with typetraits.elementType

@timotheecour
Copy link
Owner Author

timotheecour commented Nov 13, 2020

  • testament:

targets: "c c++ js"
=>
targets: "c cpp js"

(only a few c++, most are cpp)

=> nim-lang#15961

@timotheecour
Copy link
Owner Author

timotheecour commented Nov 13, 2020

  • use
    include "$nim/tests/stdlib/tdecls.nim"
    instead of relative paths starting with ..
    eg
tests/stdlib/trst.nim:9:1:import ../../lib/packages/docutils/rstgen
tests/stdlib/trst.nim:10:1:import ../../lib/packages/docutils/rst
tests/stdlib/trstgen.nim:7:1:import ../../lib/packages/docutils/rstgen
tests/stdlib/trstgen.nim:8:1:import ../../lib/packages/docutils/rst
tests/stdlib/tstrutils2.nim:1:1:import "$lib/.." / compiler/strutils2
tools/grammar_nanny.nim:6:1:import ".." / compiler / [
tools/kochdocs.nim:5:1:import "../compiler/nimpaths"
tools/nimfind.nim:33:1:import "../compiler" / [options, commands, modules, sem,
tools/winrelease.nim:7:1:import "../koch"

note:

might require: nim --prefix:PATH to set the prefix path (might be more useful than --lib:lib) · Issue #365 · timotheecour/Nim

@timotheecour
Copy link
Owner Author

timotheecour commented Nov 23, 2020

@timotheecour
Copy link
Owner Author

timotheecour commented Nov 26, 2020

  • change assert to doAssert in tests

EDIT: see nim-lang#16486 (comment) for remaining assert's to change

@timotheecour
Copy link
Owner Author

timotheecour commented Dec 29, 2020

  • add --styleCheck:error to tests/config.nims + whatever nim doc runs in CI and whitelist things where this doesn't work yet

@timotheecour
Copy link
Owner Author

@timotheecour
Copy link
Owner Author

timotheecour commented Dec 31, 2020

  • code-block:: Nim in $nim_prs_D/lib/pure/htmlparser.nim is potentially dangerous
    (involves: writeFile("output.html", $html))
    and anyways should be replaced by a (non-dangerous) runnableExamples

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 3, 2021

  • fix style for lib/js/jsffi.nim and /lib/std/private/*.nim

nim-lang/fusion#64 (comment)

because theres stuff with broken style on Nim main repo on /lib/js/jsffi.nim and /lib/std/private/*.nim.

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 4, 2021

  • fix things like: when not defined(js) and not defined(nimdoc) which are often wrong
    and replace with: when not defined(js)

refs: nim-lang/fusion#57 (comment) and nim doc -b:js

=> fixed in nim-lang#16579

@timotheecour
Copy link
Owner Author

move -d:testing to tests/config.nims so it applies to all tests even ones run via exec nim inside tests

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 4, 2021

  • change all these:
when not defined(js):
  {.error: "Module jsFFI is designed to be used with the JavaScript backend.".}

into these:

when not defined(js):
  {.fatal: "Module jsFFI is designed to be used with the JavaScript backend.".}

rationale: nim check (or nim r --errormax:10) will otherwise show tons of (irrelevant) errors otherwise (try with: nim check lib/js/jsre.nim)

In these case, early abort is correct.

note

ditto in similar other cases, eg with cpp instead of js etc

@timotheecour
Copy link
Owner Author

@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

not now

  • use math.isNaN
  if (result.n[0].kind in {nkFloatLit..nkFloat64Lit} and classify(result.n[0].floatVal) == fcNan) or
      (result.n[1].kind in {nkFloatLit..nkFloat64Lit} and classify(result.n[1].floatVal) == fcNan):
    localError(c.config, n.info, "NaN is not a valid start or end for a range")

@timotheecour
Copy link
Owner Author

  • cleanup:
    lib/pure/asynchttpserver.nim
      var (address, client) = await server.socket.acceptAddr()
      asyncCheck processClient(server, client, address, callback)

=>

      await server.acceptRequest(callback)

@timotheecour
Copy link
Owner Author

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 12, 2021

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 13, 2021

  • csize=>csize_t
    eg: posix.nim(885, 29) Warning: use csize_t instead; csize is deprecated [Deprecated]

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 13, 2021

  • use --warningAsError:UnusedImport in several places to avoid corresponding regressions:
  • in runCI during bootstrap of nim (easy)
  • in tests/config.nims (harder as may need to fix tests); note: individual tests can override via --warningAsError:UnusedImport:off, probably
  • in kochdocs.nim instead of this (harder):
nimArgs = "--errormax:3 --hint:Conf:off --hint:Path:off --hint:Processing:off --hint:XDeclaredButNotUsed:off --warning:UnusedImport:off -d:boot --putenv:nimversion=$#" % system.NimVersion

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 13, 2021

@timotheecour
Copy link
Owner Author

  • SOURCE_DATE_EPOCH should be removed/replaced

@timotheecour
Copy link
Owner Author

  • move all ffi docs (and pragmas) from manual to a separate ffi_c, ffi_cpp, ffi_js rst files

refs nim-lang#16750 (comment)

@timotheecour
Copy link
Owner Author

weak test, doesn't check that
hash(x) == static(hash(x)) in this branch

@timotheecour
Copy link
Owner Author

  • 2021-02-18T07:56:46.4635833Z D:\a\1\s\lib\impure\db_sqlite.nim(184, 1) Warning: Circular dependency detected. codeReordering pragma may not be able to reorder some nodes properely [User]

https://dev.azure.com/nim-lang/255dfe86-e590-40bb-a8a2-3c0295ebdeb1/_apis/build/builds/13280/logs/99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants