cramertj added a comment.
Looks pretty good to me-- left a couple nits inline. There are a lot of uses of `unwrap`, `expect`, and `panic` that can (and probably should) be replaced with proper error handling using `Result` (and the `failure` crate). There are also a couple of crates that provide safe bindings to Python interpreters-- I'm not sure what your external dependency situation is, but you might consider using something like https://crates.io/crates/pyo3 rather than writing your own `unsafe` calls to the python interpreter. INLINE COMMENTS > build.rs:12 > +#[cfg(target_os = "windows")] > +use std::path::PathBuf; > + Nit: if you move this import into `have_shared`, you'll only need one `cfg` and it'll be easier to validate that the proper deps are available for the proper platforms. > build.rs:88 > + > +const REQUIRED_CONFIG_FLAGS: [&'static str; 2] = ["Py_USING_UNICODE", > "WITH_THREAD"]; > + Nit: not sure what version you're targeting, but `'static` is automatic for `const` vars, so you could write `[&str; 2]` > main.rs:45 > + > + let python_exe: &'static str = env!("PYTHON_INTERPRETER"); > + let python_exe = PathBuf::from(python_exe); Nit: you can just write `&str`. Also, I'm not familiar with what you're trying to do here, but is the PYTHON_INTERPRETER always determined at compile-time? It seems like something you might want to switch on at runtime. Is that not the case? > main.rs:125 > + > + // Set program name. The backing memory needs to live for the duration > of the > + // interpreter. If it needs to live for the whole time, consider storing it in a `static` or similar. There's a subtle unsafety here: if this program panics (due to `panic`, `unwrap`, or `expect`, `program_name` will be dropped (free'd) before the python interpreter is killed (when the process ends, that is-- `Finalize` won't ever be called in that case). I don't know how much of an issue this will be in practice, but it's something to think about. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1581 To: indygreg, #hg-reviewers, yuja Cc: cramertj, yuja, quark, durin42, dlax, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
