softdevteam / grmtools

Rust grammar tool libraries and binaries
Other
494 stars 32 forks source link

Clippy fixes. #441

Closed ltratt closed 4 months ago

ratmice commented 4 months ago

Been a bit enamored with the cargo hack tool lately, so ran this through cargo hack --feature-powerset, which came back relatively clean.

It did produce one warning with cargo clippy --no-default-features -p lrtable

info: running `cargo clippy --no-default-features` on lrtable (11/20)
warning: field `final_state` is never read
   --> lrtable/src/lib/statetable.rs:144:5
    |
133 | pub struct StateTable<StorageT> {
    |            ---------- field in this struct
...
144 |     final_state: StIdx<StorageT>,
    |     ^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

I think that field being unread looks real, but is perhaps being suppressed by reads from the serde feature?

ltratt commented 4 months ago

I think that field being unread looks real, but is perhaps being suppressed by reads from the serde feature?

Ah, yes, maybe! Shall we try removing it and see if it goes through CI?

ratmice commented 4 months ago

It does actually look like it gets used in one test, but clippy can't see that because of conditional compilation of the test module. https://github.com/softdevteam/grmtools/blob/b5abf0899535d5a2814e2f05e113230783dad923/lrtable/src/lib/statetable.rs#L645

It seems reasonable to want to keep testing that, I'm not sure how I feel about adding the field conditionally during testing, any thoughts?

+   #[cfg(test)]
     final_state: StIdx<StorageT>,
+            #[cfg(test)]
             final_state: final_state.unwrap(),
ltratt commented 4 months ago

I'm very fine with that sort of conditional testing if you want to raise a PR for it!