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

progess default field #19755

Closed
wants to merge 7 commits into from
Closed

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented May 3, 2022

inspired by #12378

default fields for object

  • result
  • object variant

the addition compared to #12378

  • var
  • omit type for default fields
  • default(Type)
  • new(ref)
  • nested object definition
  • array
  • an object of array etc. in the definition of the object
  • threadvar
  • noinit
  • range
  • fieldVisible issue
  • tuple
  • distinct
  • newFinalize
  • seq(var, result, setLen)
  • docs
  • tests

notes for me only

abstractInst vs {tyGenericInst, tyAlias, tySink}

@ringabout ringabout closed this May 3, 2022
@ringabout ringabout reopened this May 3, 2022
@ringabout ringabout marked this pull request as draft May 3, 2022 05:24
@ringabout
Copy link
Member Author

It is quite complex...

@ringabout
Copy link
Member Author

ringabout commented May 5, 2022

It seems that now I need to add a flag to handle fieldVisible issue...

@ringabout
Copy link
Member Author

As a bonus, range defined in the object seem to work now(namely DateTime can be used as an object attribute) ignoring the field visible issue.

@ringabout ringabout force-pushed the pr_object_default branch 6 times, most recently from 7be8c57 to 0b32c96 Compare May 6, 2022 13:57
@ringabout ringabout mentioned this pull request May 11, 2022
33 tasks
@ringabout ringabout force-pushed the pr_object_default branch 15 times, most recently from 234f4c3 to 2cf87ec Compare May 15, 2022 15:41
@@ -502,6 +502,7 @@ type
nfLastRead # this node is a last read
nfFirstWrite# this node is a first write
nfHasComment # node has a comment
nfUseDefaultField
Copy link
Member

Choose a reason for hiding this comment

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

There is a better way, query sym.ast and see if it contains a value and not nkEmpty.

Copy link
Member Author

@ringabout ringabout Jul 4, 2022

Choose a reason for hiding this comment

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

This flag is to solve field visible problem.

  Default* = object
    poi: int = 12
    clc: Clean
    se*: range[0'i32 .. high(int32)]

poi is not a public object attribute. For a object construct in other modules, I need to distinguish default construct from manual construct.

Default() should pass while Default(poi: 15) should raise. And The ast of Default isn't empty.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand your remark. What does it mean "it should raise"?

Copy link
Member Author

Choose a reason for hiding this comment

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

The flag is used to indicate that this field has been used as the default object construction so that the semConstr won't issues an error for private fields.

For example, I have a a.nim module which has a Default object with private fields.

# a.nim
type
  Default* = object
    id: int = 1

Now I imports it in the b.nim module. var x: Default will be transformed into var x: Default = Default(id: 1). id is a private field, we need to use .ast != nil or
a new flag to let the compiler allow this.

# b.nim
import a

var x: Default

However .ast != nil doesn't work in the case that users use the id field wrongly. Even if id is a private field, users can still use it in the other modules, which causes confusion.

# b.nim
import a

var x = Default(id: 10)

Using .ast != nil cannot distinguish the two cases.

@ringabout ringabout closed this Aug 14, 2022
@ringabout
Copy link
Member Author

succeeded by #20220

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.

2 participants