TritonVM / tasm-lib

A collection of functions written in Triton VM assembly (tasm)
Apache License 2.0
11 stars 2 forks source link

Remove unneeded `Debug` trait requirement from `BasicSnippet` and `DeprecatedSnippet` #82

Open Sword-Smith opened 6 months ago

jan-ferdinand commented 6 months ago

Does this trait requirement interfere with anything? Do the #[derive(Debug)] interfere with anything? It is common practice to #[derive(Debug)] wherever possible, unless there's a good reason not to do so, of which I can find few and none that seem to apply here.

Sword-Smith commented 6 months ago

Does this trait requirement interfere with anything? Do the #[derive(Debug)] interfere with anything? It is common practice to #[derive(Debug)] wherever possible, unless there's a good reason not to do so, of which I can find few and none that seem to apply here.

I see two arguments for removing Debug

  1. It puts a Debug trait constraint on fields in snippet structs that implement BasicSnippet
  2. It's not needed anywhere as a snippet should be fully identified by its entrypoint. So adding it gives the compiler more work than necessary.
jan-ferdinand commented 6 months ago
  1. I return to my first question; does the trait bound interfere with anything?
  2. I'd be very surprised if that additional workload was significant.

In the end, this is your codebase; do as you see fit.

Sword-Smith commented 6 months ago
  1. I return to my first question; does the trait bound interfere with anything?

    1. I'd be very surprised if that additional workload was significant.

In the end, this is your codebase; do as you see fit.

  1. Not currently. I could, theoretically, in the future.
Sword-Smith commented 6 months ago

I guess my opinion is more based on aesthetics and how I think you should inspect values of this type. Aesthetically I don't like to impose trait bounds that are not needed, and I think that you should inspect the the type of a snippet through its entrypoint name, not through its debug output.

jan-ferdinand commented 6 months ago

If that is the case, then how about the following?


impl Debug for dyn BasicSnippet {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.entrypoint())
    }
}

This would also allow the #[derive(Debug)] on the various types to stay in place.

Sword-Smith commented 6 months ago

Your suggestion works. But I should also like to point out that no Debug derivations are removed with this PR, it's only the requirement that is removed.

jan-ferdinand commented 6 months ago

What about removal of line 35 in all.rs, line 31 in filter.rs, line 83 in inner_function.rs, and line 37 of map.rs?

Sword-Smith commented 6 months ago

What about removal of line 35 in all.rs, line 31 in filter.rs, line 83 in inner_function.rs, and line 37 of map.rs?

You're right. It got removed from higher-order snippets.