Due to limitations imposed by the borrow checker, there are very few places
where we will be able to use the `ScopedPush` class ported over from the C++
codebase (once you capture the value w/ a `ScopedPush` you can't access the
value - or the mutable reference you used to reach it! - until the `ScopedPush`
object goes out of scope).
This alternative requires binding the previous values to a variable and manually
restoring them in the callback passed to the `ScopeGuard` constructor, but will
work with rust's borrow and `&mut` paradigm.
This was added to support signals; however we are unlikely to use this
for anything else. Remove it; just use a u64 to report signals that have
been set.
This optimizes over both the rust rewrite and the original C++ code. The rust
rewrite saw `std::bitset` replaced with `[bool; 65]` which could result in a
lot of memory copy bandwidth each time we checked for and received no signals.
The original C++ code would iterate over all signal slots to see if any were
set. The code now returns a single u64 and only checks slots that are known to
have signals via an intelligent `Iterator` impl.
You can now use a reference to CxxWString or an allocated UniquePtr<CxxWString>
to get an &wstr temporary to use without having to allocate again (e.g. via
`from_ffi()`).
wchar.rs should not import let alone reexport FFI strings.
Stop re-exporting utf32str! because we use L! instead.
In wchar_ffi.rs, stop re-exporting cxx::CxxWString because that hasn't
seen adoption.
I think we should use re-exports only for aliases like "wstr" or for aliases
into internal modules.
So I'd probably remove `pub use wchar_ffi::wcharz_t = crate::ffi::wcharz_t`
as well.
bool_assert_comparison is stupid, the reason they give is "it's shorter". Well,
`assert!(!foo)` is nowhere near as readable as `assert_eq!(foo, false)` because
of the ! noise from the macro.
Uninlined format args is a stupid lint that Rust actually walked back when they
made it an official warning because you still have to use a mix of inlined and
un-inlined format args (the latter of which won't complain) since only idents
can be inlined.
This shows some of the ugliness of the rust borrow checker when it comes to
safely implementing any sort of recursive access and the need to be overly
explicit about which types are actually used across threads and which aren't.
We're forced to use an `Arc` for `ItemMaker` (née `item_maker_t`) because
there's no other way to make it clear that its lifetime will last longer than
the FdMonitor's. But once we've created an `Arc<T>` we can't call
`Arc::get_mut()` to get an `&mut T` once we've created even a single weak
reference to the Arc (because that weak ref could be upgraded to a strong ref at
any time). This means we need to finish configuring any non-atomic properties
(such as `ItemMaker::always_exit`) before we initialize the callback (which
needs an `Arc<ItemMaker>` to do its thing).
Because rust doesn't like self-referential types and because of the fact that we
now need to create both the `ItemMaker` and the `FdMonitorItem` separately
before we set the callback (at which point it becomes impossible to get a
mutable reference to the `ItemMaker`), `ItemMaker::item` is dropped from the
struct and we instead have the "constructor" for `ItemMaker` take a reference to
an `FdMonitor` instance and directly add itself to the monitor's set, meaning we
don't need to move the item out of the `ItemMaker` in order to add it to the
`FdMonitor` set later.
We were only using their ffi implementations which are automatically
exported/public, but the actual functions we would need if we were to use
FdMonitor and co. in native rust code were either private or missing convenient
wrappers.
The existing code is kept, but a rusty version of these functions is added for
code that needs them.
These should only be temporarily used when porting 1-to-1 from C++; we should
use the std library's `read()` and `write_all()` methods instead in the future.
By extracting the equivalent of i32::cmp() into its own const function,
it becomes a lot easier to see what is happening and the logic can be
more direct.
These will be used in the parser.
Maybe this type should be a struct with boolean fields. The current way has
the upside that the usage is exactly the same as in C++.
For some reason this error is triggered by tests after the Rust port of
ast.cpp. Might want to get to the bottom of this but moving it back
to match the original C++ logic fixes it.
Prior to this fix, the Rust FLOG output was regressed from C++, because
it put quotes around strings. However if we used Display, we would fail
to FLOG non-display types like ThreadIDs.
There is apparently no way in Rust to write a function which formats a
value preferentially using Display, falling back to Debug.
Fix this by introducing two new traits, FloggableDisplay and
FloggableDebug. FloggableDisplay is implemented for all Display types,
and FloggableDebug can be "opted into" for any Debug type:
impl FloggableDebug for MyType {}
Both traits have a 'to_flog_str' function. FLOG brings them both into
scope, and Rust figures out which 'to_flog_str' gets called.
* wutil: Rewrite `wrealpath` in Rust
* Reduce use of FFI types in `wrealpath`
* Addressed PR comments regarding allocation
* Replace let binding assignment with regular comparison
More ugliness with types that cxx bridge can't recognize as being POD. Using
pointers to get/set `termios` values with an assert to make sure we're using
identical definitions on both sides (in cpp from the system headers and in rust
from the libc crate as exported).
I don't know why cxx bridge doesn't allow `SharedPtr<OpaqueRustType>` but we can
work around it in C++ by converting a `Box<T>` to a `shared_ptr<T>` then convert
it back when it needs to be destructed. I can't find a clean way of doing it
from the cxx bridge wrapper so for now it needs to be done manually in the C++
code.
Types/values that are drop-in ready over ffi are renamed to match the old cpp
names but for types that now differ due to ffi difficulties I've left the `_ffi`
in the function names to indicate that this isn't the "correct" way of using the
types/methods.
We want to keep the cast because tv_sec is not always 64 bits, see b5ff175b4
(Fix timer.rs cross-platform compilation, 2023-02-14).
It would be nice to avoid the clippy exemption, perhaps using something like
#[cfg(target_pointer_width = "32")]
let seconds = val.tv_sec as i64;
#[cfg(not(target_pointer_width = "32"))]
let seconds = val.tv_sec;
but I'm not sure if "target_pointer_width" is the right criteria.
Upsizing to `usize` from `i32` doesn't work if `usize` is only 32-bits.
I changed the code to use the `FromStr` impl on `i32`, but we could have also
just used `u64` instead of `i32`.
Also, we should get in the habit of using the appropriate type aliases where
possible (`i32` should be `RawFd`).
The mutex was being locked from the very start, before it was needed and
possibly before it would be needed.
Also rename the static global to stick to rust naming conventions.
Note that `once_cell::sync::Lazy<T>` actually internally uses its own lock
around the value, but in this case it's insufficient because `SmallRng` doesn't
implement `SeedableRng` so we can't reseed it with only an `&mut` reference and
must instead replace its value.
We probably *could* still use `Lazy<SmallRng>` directly and then rely on
`std::mem::swap()` to replace the contents of the shared global static without
reassigning the variable directly with a new `SmallRng` instance, but I'm not
sure that's a great idea. This is just a built-in, there's no real harm in
locking twice (especially while fish remains essentially single-threaded).
The old comments about using i128 logic were still there even though we are no
longer using that approach and the output type was very much misleadingly a u64
printed to the console (but via `%d` so it was ultimately shown as an i64). Be
explicit about the resulting being a valid i64 value before passing it to the
sprintf!() macro.
Also add comments about the safety of the final `unwrap()` operation.
Rust doesn't have __FUNCTION__ or __func__ (though you can hack around it with a
proc macro, but that will require a separate crate and slowing down compilation
times with heavy proc macro dependencies), so these are just regular functions
(at least for now). Rust's default stack trace on panic (even in release mode)
should be enough (and the functions themselves are inlined so the calling
function should be the second frame from the top, after the #[cold] panic
functions).
This is to allow us to verify some implementation details that aren't explicitly
documented in the rust standard library's documentation.
std::thread uses `pthread_create()` underneath the hood on *nix platforms, so
this *should* merely be a formality.
The way cxx bridge works, it doesn't recognize any types from another module as
being shared cxx bridge types with generations native to both C++ and Rust,
meaning every module that was going to use function pointers would have to
define its own `c_void` type (because cxx bridge doesn't recognize any of
libc::c_void, std::ffi::c_void, or autocxx::c_void).
FFI on other platforms has long used the equivalent of `uint8_t *` as an
alternative to `void *` for code where `void` was not available or was
undesirable for some reason. We can join the club - this way we can always use
`* {const|mut} u8` in our rust code and `uint8_t *` in our C++ code to pass
around parameters or values over the C abi.
I needed to rename some types already ported to rust so they don't clash with
their still-extant cpp counterparts. Helper ffi functions added to avoid needing
to dynamically allocate an FdMonitorItem for every fd (we use dozens per basic
prompt).
I ported some functions from cpp to rust that are used only in the backend but
without removing their existing cpp counterparts so cpp code can continue to use
their version of them (`wperror` and `make_detached_pthread`).
I ran into issues porting line-by-line logic because rust inverts the behavior
of `std::remove_if(..)` by making it (basically) `Vec::retain_if(..)` so I
replaced bools with an explict enum to make everything clearer.
I'll port the cpp tests for this separately, for now they're using ffi.
Porting closures was ugly. It's nothing hard, but it's very ugly as now each
capturing lambda has been changed into an explicit struct that contains its
parameters (that needs to be dynamically allocated), a standalone callback
(member) function to replace the lambda contents, and a separate trampoline
function to call it from rust over the shared C abi (not really relevant to
x86_64 w/ its single calling convention but probably needed on other platforms).
I don't like that `fd_monitor.rs` has its own `c_void`. I couldn't find a way to
move that to `ffi.rs` but still get cxx bridge to consider it a shared POD.
Every time I moved it to a different module, it would consider it to be an
opaque rust type instead. I worry this means we're going to have multiple
`c_void1`, `c_void2`, etc. types as we continue to port code to use function
pointers.
Also, rust treats raw pointers as foreign so you can't do `impl Send for * const
Foo` even if `Foo` is from the same module. That necessitated a wrapper type
(`void_ptr`) that implements `Send` and `Sync` so we can move stuff between
threads.
The code in fd_monitor_t has been split into two objects, one that is used by
the caller and a separate one associated with the background thread (this is
made nice and clean by rust's ownership model). Objects not needed under the
lock (i.e. accessed by the background thread exclusively) were moved to the
separate `BackgroundFdMonitor` type.
We should fix this warning eventually. Silence it for now to make Clippy
pass without warnings, which makes it much more useful.
Compiling fish-rust v0.1.0 (/home/johannes/git/fish-riir/fish-rust)
error: mutable borrow from immutable input(s)
--> src/ffi.rs:79:32
|
79 | pub fn get_procs(&self) -> &mut [UniquePtr<process_t>] {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: immutable borrow here
--> src/ffi.rs:79:22
|
79 | pub fn get_procs(&self) -> &mut [UniquePtr<process_t>] {
| ^^^^^
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref
= note: `#[deny(clippy::mut_from_ref)]` on by default
error: could not compile `fish-rust` due to previous error
A following commit will pass global string constants to the gettext macro.
This is not ideal because we might accidentally use the constants without
gettext (which we should never do). To fix that we might need to define a
macro per constant, or use a proc macro which is maybe not worth it.
warning: deref which would be done by auto-deref
--> src/wchar_ffi.rs:81:5
|
81 | &*EMPTY_WSTRING
| ^^^^^^^^^^^^^^^ help: try this: `&EMPTY_WSTRING`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref
= note: `#[warn(clippy::explicit_auto_deref)]` on by default
lazy_static has better ergonomics at the call/access sites (it returns a
reference to the type directly, whereas with once_cell we get a static Lazy<T>
that we must dereference instead) but the once_cell api is slated for
integration into the standard library [0] and has been the "preferred" way to
declare static global variables w/ deferred initialization. It's also less
opaque and easier to comprehend how it works, I guess?
(Both `once_cell` and `lazy_static` are already in our dependency tree, so this
should have no detrimental effect on build times. It actually negligibly
*improves* build times by not using macros, reducing the amount of expansion the
compiler has to do by a miniscule amount.)
[0]: https://github.com/rust-lang/rust/issues/74465
It's debatable whether is_ascii_digit() is better than (0..=9).contains().
(Probably we want to go with the mainstream Rust choice eventually.)
Let's disable the warning for now since it's not terribly important.
We should only be dealing with wcharz_t at the language boundary.
Rust callers should prefer the equivalent &wstr.
Since wcsfilecmp() is no longer exposed directly it can take &wstr only.
This is early work but I guess there's no harm in pushing it?
Some thoughts on the conventions:
Types that live only inside Rust follow Rust naming convention
("FeatureMetadata").
Types that live on both sides of the language boundary follow the existing
naming ("feature_flag_t").
The alternative is to define a type alias ("using feature_flag_t =
rust::FeatureFlag") but that doesn't seem to be supported in "[cxx::bridge]"
blocks. We could put it in a header ("future_feature_flags.h").
"feature_metadata_t" is a variant of "FeatureMetadata" that can cross
the language boundary. This has the advantage that we can avoid tainting
"FeatureMetadata" with "CxxString" and such. This is an experimental approach,
probably not what we should do in general.
The original implementation without the test took me 3 hours (first time
seriously looking into this)
The functions take "wcharz_t" for smooth integration with existing C++ callers.
This is at the expense of Rust callers, which would prefer "&wstr". Would be
nice to declare a function parameter that accepts both but I don't think
that really works since "wcharz_t" drops the lifetime annotation.
rustfmt removes the "::" prefix from qualifiers. This breaks the build because
I think a later "pub use ffi::*" results in "std" being an ambiguous reference.
This adds an implementation of fish_wcstoi in Rust, mirroring the one in
fish. As Rust does not have a string to number which infers the radix
(i.e. looks for leading 0x or 0), we add that manually.
