chipsalliance / fpga-interchange-schema

https://fpga-interchange-schema.readthedocs.io/
Apache License 2.0
51 stars 20 forks source link

Inconsistent use of Text vs StringIdx #46

Open gatecat opened 3 years ago

gatecat commented 3 years ago

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 commented 3 years ago

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

gatecat commented 3 years ago

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

mithro commented 3 years ago

@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 commented 3 years ago

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

mithro commented 3 years ago

@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;
}