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

improvements to addInt and $ for integer types #18592

Merged
merged 27 commits into from
Aug 19, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 26, 2021

  • major simplifications to $ and addInt for integer types: instead of hardcoding the logic in the compiler (spread over a whopping > 10 files, with magics, compilerProcs, etc), all the logic is now handled in 1 place for addInt (digitsutils) and 1 place for $: (dollars)
  • fix $ is wrong for unsigned types (eg uint8) in js #18591: $ and addInt for js in runtime now works correctly by calling js x + "" in js backend (consistent with what console.log would return), instead of producing wildly inconsistent and non-sensical results
  • addInt now supports unsigned integer types, avoiding the slower foo.add $x which allocates (see 3x faster digits10 via binary search (speeds up $ for numbers) #18324 which has benchmarks showing, unsurprisingly, that those allocations are costly)
  • improve implementation of toLocation (avoid allocations, and avoid when declared)
  • avoid allocations in addQuoted for unsigned ints

note:

splitting these tasks in separate PRs would complicate things a lot in this case, due to a number of constraints (in particular some bugs in csources_v1 that prevent certain obvious things from working)

@timotheecour timotheecour force-pushed the pr_addInt_uint branch 3 times, most recently from e7aaa7c to e98b337 Compare July 29, 2021 09:35
@timotheecour timotheecour marked this pull request as ready for review July 29, 2021 10:48
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jul 29, 2021
@timotheecour timotheecour requested a review from Araq July 29, 2021 21:32
tests/system/tstrmantle.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

PTAL

@timotheecour
Copy link
Member Author

timotheecour commented Aug 13, 2021

@Araq ping on this before this bitrots

@Araq
Copy link
Member

Araq commented Aug 16, 2021

Nim devel cannot compile Nim 1.4 or earlier because you removed the magics. This removes more magics and so the problem will be worse. Would be nice if we could think of a solution for this problem; apart from that, I like the changes.

@timotheecour
Copy link
Member Author

timotheecour commented Aug 16, 2021

indeed, after #18531, nim devel can't compile 1.4.0:

dollars.nim(27, 36) Warning: unknown magic 'FloatToStr' might crash the compiler [UnknownMagic]
dollars.nim(27, 6) Error: implementation of 'dollars.$(x: float)' expected

note that even before #18531, nim devel can't compile 1.2.0:

system.nim(164, 8) Warning: unknown magic 'DefinedInScope' might crash the compiler [UnknownMagic]
system.nim(163, 6) Error: implementation of 'system.declaredInScope(x: untyped)' expected

requiring that nim devel can compile prior versions prevents much needed simplifications though; plenty of magics (eg abs) should in fact be turned into regular procs (and in some cases vmops), which avoids spreading the implementation over all backends (c, js, vm); it wouldn't be so bad if magics implementation were self-contained, but they're spread over many places, eg in this PR the magic implementations I removed were spread over 10 files and probably can't be simplified if I keep them in (likewise, implementation of $float was spread over > 10 files before #18531)

And, because compilining 1.4 from devel is not tested in CI, other PRs further break this (eg #11955 which introduces a new compilerProc raiseFieldError2).

Note that you already have the means to programmatically find the right nim version with which to build any prior nim version >= #17815 by checking in the bootstrap url+commit hash, and, with nimdigger (#18119), we can build any nim version >= v0.12.0

In short, I'd rather keep simplifying the compiler than require that new nim versions be able to build older nim versions, which adds un-necessary complexity.

@arnetheduck
Copy link
Contributor

In short, I'd rather keep simplifying the compiler than require that new nim versions be able to build older nim versions, which adds un-necessary complexity.

the bare minimum standard here tends to be that each version of the compiler can be compiled by the previous version of the compiler - this allows people with a working compiler to upgrade it to the next version without leaving the language - applying that strategy recursively one can reach any released version from any earlier version.

It's somewhat surprising that compiling older versions should be a problem if basic backwards compatibility strategies are adhered to (ie deprecating instead of removing being the most simple one), but it's of course easier to not think about this.

@timotheecour
Copy link
Member Author

timotheecour commented Aug 16, 2021

the bare minimum standard here tends to be that each version of the compiler can be compiled by the previous version of the compiler

nim 1.4 can compile nim devel, both before and after this PR.

It's somewhat surprising that compiling older versions should be a problem if basic backwards compatibility strategies are adhered to (ie deprecating instead of removing being the most simple one),

this doesn't impact backward compatibility for user code

@timotheecour
Copy link
Member Author

ping @Araq

@Araq
Copy link
Member

Araq commented Aug 18, 2021

Keep the magics, I enjoy being able to compile 1.4 with Nim devel. (This implies the other removed magics should be brought back too.)

@timotheecour
Copy link
Member Author

=> #18708

@timotheecour
Copy link
Member Author

timotheecour commented Aug 18, 2021

Keep the magics

@Araq done, PTAL

@Araq Araq merged commit 394f4ac into nim-lang:devel Aug 19, 2021
@timotheecour timotheecour deleted the pr_addInt_uint branch August 19, 2021 15:48
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* improvements to $(SomeInteger) and addInt
* remove mIntToStr, mInt64ToStr
* improvements
* fix tests/pragmas/tinjectstmt.nim; the diff is harmless, cgen code is identical with -d:danger or debug mode
* rm tests/system/tstrmantle.nim
* revert compiler/jsgen.nim for -d:nimVersion140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$ is wrong for unsigned types (eg uint8) in js
4 participants