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.

Sign up for GitHub

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

Jump to bottom

Inconsistent use of Text vs StringIdx #46

Open
gatecat opened this issue Apr 27, 2021 · 5 comments
Open

Inconsistent use of Text vs StringIdx #46

gatecat opened this issue Apr 27, 2021 · 5 comments

Comments

@gatecat
Copy link
Collaborator

gatecat commented Apr 27, 2021

Almost all references to strings, even those that won't use much storage space in practice like constant net and cell names, are using the indexed string representation StringIdx.

However, the Constraints and LutDefinitions sections are currently using Text for all their strings. I suspect this was originally a hack to make patching these in easier, however the proper solution here seems to be to support indexed strings in the patching tool.

Unfortunately this can't now be fixed without a breaking change, but it's worth considering going forward (a) whether we want to break and fix this, and (b) how we approach similar things in the future, e.g. in #45 .
@mithro
Copy link
Contributor

mithro commented Apr 28, 2021

@gatecat - On the migration strategy, I think we just add new fields and mark the old ones as obsolete?

@gatecat
Copy link
Collaborator Author

gatecat commented Apr 28, 2021

That's going to result in a lot of duplicated fields and complexity for stuff that accesses it, unfortunately.

@mithro
Copy link
Contributor

mithro commented Apr 28, 2021

@gatecat - Since we only have a very limited number of users at the moment, I think we just use all start using the new fields and leave the old fields as unused?

@gatecat
Copy link
Collaborator Author

gatecat commented Apr 28, 2021

At that point, it's probably easier just to remove the old fields and avoid having a source of confusion left in there?

@mithro
Copy link
Contributor

mithro commented Apr 28, 2021

@gatecat We should be able to do something like this?

// Before
struct Date {
  field1 @0 :Int16;
  field2 @1 :UInt8;
  field3 @2 :UInt8;
}

// After
struct Date {
  _unused1 @0 :Int16; // Was field1 but has been removed.
  field2 @1 :UInt8;
  field3 @2 :UInt8;
  new_field1 @3 :Float64;
}

@gatecat gatecat mentioned this issue May 10, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mithro @gatecat