-
Notifications
You must be signed in to change notification settings - Fork 21
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
Inconsistent use of Text vs StringIdx #46
Comments
@gatecat - On the migration strategy, I think we just add new fields and mark the old ones as obsolete? |
That's going to result in a lot of duplicated fields and complexity for stuff that accesses it, unfortunately. |
@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? |
At that point, it's probably easier just to remove the old fields and avoid having a source of confusion left in there? |
@gatecat We should be able to do something like this?
|
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
andLutDefinitions
sections are currently usingText
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 .
The text was updated successfully, but these errors were encountered: