search

oooutlk / tcltk

Rust bindings to Tcl/Tk commands
Apache License 2.0
18 stars 3 forks source link

Add package_provide method to Interpreter #8

Closed benburwell closed 1 year ago

benburwell commented 1 year ago

This exposes a hook for the Tcl_PkgProvide C API, and allows callers to expose Rust functionality as a Tcl package.

oooutlk commented 1 year ago

Thanks for your pr! Two questions:

  1. Why package_provide() is marked as unsafe? And a "safety" section is required in the API doc comment.
  2. The corresponding TCL command is "package provide package ?version?". Should we make the version argument an Option<&str>, or provide two APIs, e.g. package_provide() and package_provide_version()?
oooutlk commented 1 year ago

A call to Interp::eval(( “package”, “provide”, “thepkgname”, “thepkgversion” )) will do the same thing as Interp::package_provide( “thepkgname”, “thepkgversion” ), and it’s safe. I think they should be of the same safety, but I’m not sure if they should be marked as safe or unsafe. See https://internals.rust-lang.org/t/should-eval-be-marked-as-unsafe-fn/18551 for more.

benburwell commented 1 year ago

Thanks for the feedback!

That's an interesting thread about unsafe. I'm fairly new to Rust, but based on that discussion it sounds like we don't have to mark package_provide as unsafe, because even though it calls into a C implementation, we don't expect that calling it with any values would cause undefined behavior. This seems to align with other methods already in interp.rs as well.

The corresponding TCL command is "package provide package ?version?". Should we make the version argument an Option<&str>, or provide two APIs, e.g. package_provide() and package_provide_version()?

The behavior of package provide name ?version? is quite different when version is not specified. From the manual page: "If the version argument is omitted, then the command returns the version number that is currently provided, or an empty string if no package provide command has been invoked for package in this interpreter."

I checked the source for Tcl_PkgProvide to see if it works that way too, and it appears that it requires version to be a valid version string by calling CheckVersionAndConvert on line 177.

So I think that whether we should make the version optional depends on whether we want to match the Tcl_PkgProvide behavior or the package provide name ?version? behavior. Personally I'm inclined to match the Tcl_PkgProvide behavior, but I'm curious to hear your thoughts.

oooutlk commented 1 year ago

At the moment I'm inclined to match the Tcl_PkgProvide behavior, too. I will merge the pr if unsafe is removed.