Remove identifier from bit string and guarantee lower case

oscargus commented

2023-10-04 14:28:04 +00:00

(Migrated from github.com)

Turns out that the bit string was stored including the identifier.

Also, it is better from an end user perspective that the string is always in lower or upper case, independent of the context in the VCD. Here I arbitrarily selected lower case (which is what Surfer assumes).

Turns out that the bit string was stored including the identifier. Also, it is better from an end user perspective that the string is always in lower or upper case, independent of the context in the VCD. Here I arbitrarily selected lower case (which is what Surfer assumes).

TheZoq2 commented

2023-10-05 11:59:29 +00:00

(Migrated from github.com)

I'm now realizing that .to_lowercase will probably break if there are VCD files with strings in them which as far as I know is supported

I'm now realizing that `.to_lowercase` will probably break if there are VCD files with strings in them which as far as I know is supported

oscargus commented

2023-10-05 14:44:54 +00:00

(Migrated from github.com)

But I think this only convert bit strings, i.e., if they start with b. Not sure how strings are encoded though...

oscargus commented

2023-10-06 06:36:10 +00:00

(Migrated from github.com)

Strings are parsed here: bc7c6913ce/src/vcd/parse/events.rs (L493)

so that is not affected by this PR.

Strings are parsed here: https://github.com/ThePerfectComputer/FastWaveBackend/blob/bc7c6913cefef78e5a4c8f1bb7895573bfae53c9/src/vcd/parse/events.rs#L493 so that is not affected by this PR.

TheZoq2 commented

2023-10-06 08:01:01 +00:00

(Migrated from github.com)

Great! Just one more thing for performance: you should probably use .to_ascii_lowercase(). And since both return String, you probably don't need the initial to_string

Great! Just one more thing for performance: you should probably use `.to_ascii_lowercase()`. And since both return `String`, you probably don't need the initial `to_string`

oscargus commented

2023-10-06 08:53:59 +00:00

(Migrated from github.com)

Fixed!

ThePerfectComputer commented

2023-10-06 10:14:04 +00:00

(Migrated from github.com)

These are just for non-string type signals right? That is, a state machine string signal should still retain its arbitrary cases for example?

TheZoq2 commented

2023-10-06 10:43:51 +00:00

(Migrated from github.com)

Huh, are state signals not encoded as strings?

ThePerfectComputer commented

2023-10-06 10:44:52 +00:00

(Migrated from github.com)

I should say non-value signals. So X, U, Z, etc...

ThePerfectComputer commented

2023-10-06 10:45:20 +00:00

(Migrated from github.com)

String such as "STATE_start" and "STATE_stop" should be left untouched IMO

TheZoq2 commented

2023-10-06 10:49:36 +00:00

(Migrated from github.com)

I agree, but I also think this is what this PR does, no? As Oscar said above arbitrary string parsing happens here right? bc7c6913ce/src/vcd/parse/events.rs (L493)

I might also be completely misunderstanding how this code works :D

I agree, but I also think this is what this PR does, no? As Oscar said above arbitrary string parsing happens here right? https://github.com/ThePerfectComputer/FastWaveBackend/blob/bc7c6913cefef78e5a4c8f1bb7895573bfae53c9/src/vcd/parse/events.rs#L493 I might also be completely misunderstanding how this code works :D

oscargus commented

2023-10-06 12:03:17 +00:00

(Migrated from github.com)

This PR is only for bits and bit vectors (or whatever the formal name is), yes. Strings are still untouched (as far as I understand the code). The benefit is that the consumer (in this case Surfer) does not have to convert to lower case or check double case variants. As it most likely happens more often in the consumer I thought it made sense to make this change in the provider.

Remove identifier from bit string and guarantee lower case #19