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

fixes #20681; add efSkipFieldVisibilityCheck to skip check #20639

Merged
merged 10 commits into from
Oct 28, 2022

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Oct 24, 2022

fixes #20681

Constant object construction has been checked and inlined already, there is no need to sem it again, especially for default fields.

fixes

import std/tables
block:
  type TokenData = object
    campaignMemberships = Table[string, string]()

  let x = default(TokenData)
  doAssert x.campaignMemberships.len == 0

compiler/semobjconstr.nim Outdated Show resolved Hide resolved
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Oct 24, 2022
@ringabout ringabout changed the title don't sem const objectConstr defaults add efSkipFieldVisibilityCheck to skip check; fixes the type of nkBracket Oct 25, 2022
@ringabout ringabout changed the title add efSkipFieldVisibilityCheck to skip check; fixes the type of nkBracket add efSkipFieldVisibilityCheck to skip check; fixes the type of nkBracket (seqs) Oct 25, 2022
compiler/semexprs.nim Outdated Show resolved Hide resolved
@ringabout
Copy link
Member Author

ringabout commented Oct 26, 2022

It seems that there is probably nothing wrong with this patch.

I have spent a bad time debugging the macros of loopfusion, I think these changes may work.

image

With this patch, the type of tySequence is not erased for nkBracket, which causes loopfusion to stop working.

Am I correct or do I miss something?

@ringabout ringabout marked this pull request as draft October 26, 2022 03:23
@ringabout ringabout changed the title add efSkipFieldVisibilityCheck to skip check; fixes the type of nkBracket (seqs) add efSkipFieldVisibilityCheck to skip check Oct 28, 2022
@ringabout ringabout changed the title add efSkipFieldVisibilityCheck to skip check fixes #20681; add efSkipFieldVisibilityCheck to skip check Oct 28, 2022
@ringabout
Copy link
Member Author

wait for #20662

@Araq Araq marked this pull request as ready for review October 28, 2022 10:41
@ringabout ringabout closed this Oct 28, 2022
@ringabout ringabout reopened this Oct 28, 2022
@Araq Araq added merge_when_passes_CI mergeable once green and removed Requires Araq To Merge PR should only be merged by Araq labels Oct 28, 2022
@Varriount Varriount merged commit 141abb7 into devel Oct 28, 2022
@Varriount Varriount deleted the pr_fields_const branch October 28, 2022 20:19
@github-actions
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
164440 lines; 9.829s; 613.664MiB peakmem

capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
…im-lang#20639)

* don't sem const objectConstr defaults

* fixes

* add `efSkipFieldVisibilityCheck`; fixes nkBracket types

* fixes nim-lang#20681

* fixes tests

* suggestion from @metagn

* fixes tests

Co-authored-by: xflywind <[email protected]>
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
…im-lang#20639)

* don't sem const objectConstr defaults

* fixes

* add `efSkipFieldVisibilityCheck`; fixes nkBracket types

* fixes nim-lang#20681

* fixes tests

* suggestion from @metagn

* fixes tests

Co-authored-by: xflywind <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not use nim 2's new default instantiation with any object type with a DateTime field
3 participants