search

ThePerfectComputer / FastWaveBackend

A Rust VCD parser intended to be the backend of a Waveform Viewer(built using egui) that supports dynamically loaded rust plugins.
GNU General Public License v3.0
42 stars 5 forks source link

Remove identifier from bit string and guarantee lower case #19

Closed oscargus closed 1 year ago

oscargus commented 1 year ago

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 1 year ago

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 1 year ago

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

oscargus commented 1 year ago

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 1 year ago

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 1 year ago

Fixed!

ThePerfectComputer commented 1 year ago

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 1 year ago

Huh, are state signals not encoded as strings?

ThePerfectComputer commented 1 year ago

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

ThePerfectComputer commented 1 year ago

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

TheZoq2 commented 1 year ago

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 1 year ago

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.