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

use cbuilder for tuple/object generation #24145

Merged
merged 15 commits into from
Sep 27, 2024

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 21, 2024

based on #24127

Needs some tweaks to replace the other struct type generations, e.g. seqs, maybe by exposing BaseTypeKind as a parameter. C++ and codegenDecl etc seem like they are going to need attention.

Also Builder should really be distinct string that one has to call extract on, but for this to be optimal in the current codegen, we would need something like:

template buildInto(s: var string, builderName: untyped, body) =
  template `builderName`: untyped = Builder(s)
  body

buildInto(result, builder):
  builder.add ...

but this could be a separate PR since it might not work with the compiler. The possibly-not-optimal alternative is to do:

template build(builderName: untyped, body): string =
  var `builderName` = Builder("")
  body
  extract(`builderName`)

result = build(builder):
  builder.add ...

where the compiler maybe copies the built string but shouldn't.

@Araq
Copy link
Member

Araq commented Sep 21, 2024

Also Builder should really be distinct string that one has to call extract on ...

Correct, but this will be done much later, the goal for now is tiny refactorings that keep the CI clean at all time until we're confident enough that we can replace the implementation with something else.

@Araq Araq merged commit 2cdc0e9 into nim-lang:devel Sep 27, 2024
19 checks passed
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 2cdc0e9

Hint: mm: orc; opt: speed; options: -d:release
174567 lines; 8.368s; 654.641MiB peakmem

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.

3 participants