Prior to this commit, there was a stack of ReaderDatas, each one has a
reference to a Parser (same Parser in each, for now). However, the current
ReaderData is globally accessible. Because it holds a Parser, effectively
anything can run fish script; this also prevents us from making the Parser
&mut.
Split these up. Create ReaderData, which holds the data portion of the
reader machinery, and then create Reader which holds a ReaderData and a
Parser. Now `reader_current_data()` can only return the data itself; it
cannot execute fish script.
This results in some other nice simplifications.
This is a start on untangling input. Prior to this, a ReaderData and an
Inputter would communicate with each other; this is natural in C++ but
difficult in Rust because the Reader would own an Inputter and therefore
the Inputter could not easily reference the Reader. This was previously
"resolved" via unsafe code.
Fix this by collapsing Inputter into Reader. Now they're the same object!
Migrate Inputter's logic into a trait, so we get some modularity, and then
directly implement the remaining input methods on ReaderData.
Add round options, but I think can also add floor, ceiling, etc. And
the default mode is trunc.
Closes#9117
Co-authored-by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
The special input functions self-insert, self-insert-not-first, and
and or used to be handled by inputter_t::readch, but they aren't
anymore with `commandline -f`.
I am unsure if these *would* have worked, I can't come up with a use.
So, for now, do nothing instead of panicking.
This would crash if you ran `commandline -f backward-jump`.
The C++ version would read a char (but badly), this doesn't anymore.
So, at least instead of crashing, just do nothing.
This doesn't pull its weight. Block size is not a particularly big
problem,
and this both complicates the code a bit and would arbitrarily cause issues
if a fish script exceeded 65k lines.
This reverts commit edd6533a14.
This doesn't have any effect on the size of the struct (due to alignment
requirements and padding) but reduces the complexity by turning
Block::wants_pop_env into an emergent property dependent on the type rather than
something we have to manually manage.
We only increment it and check if it's non-zero, we never decrement or check the
actual count. As such, change it to a bool and bring the size of `Block` down
from 32 to 24 bytes.
We almost never access any of this and having it stored directly in the `Block`
struct increases its size (reducing how many we can fit in L1 and L2, and
increasing memory copy traffic).
Gets rid of BlockData::None so we can avoid allocating a Box at all when we have
no data (at the cost of yet-another-wrapper-type), which is the usual case.
This has a few advantages,
* We now statically assert that all fields used by a particular block type are
correctly initialized (i.e. you can't assign the function name but forget to
assign its arguments),
* Conversely, we can match directly on `BlockData` and be guaranteed that the
fields we want to access are initialized and present,
* We reduce the number of assertions, effectively "unwrapping" only once based
off the block type instead of each time we try to access a conditional field,
* We reduce the size of the `Block` struct by coalescing fields that cannot
co-exist, bringing it down from 104 bytes to 88 bytes.
It would be nice to make all of `Block` itself an enum, but it currently
requires `Copy` and we take advantage of that to copy it around everywhere.
Putting these fields directly in `Block` directly would mean a lot more memory
traffic just checking block types.
There's no need for two separate block types when one is merely a variant of the
other. This may have been required under C++ but thanks to sum types (rust's
enums) we don't need to do that any more.
If the backgrounded/stopped job was using the tty, sending it SIGCONT first
might cause it to immediately wake and try to use the tty (which fish still has
control over), causing it to immediately stop again after receiving a SIGTTOU.
We are supposed to send SIGHUP first so that when the process resumes it sees
the queued SIGHUP and executes its registered handler!
There's no guarantee that a condition variable is stateful. The docs for
`Condvar::notify_one()` actually say the opposite:
> If there is a blocked thread on this condition variable, then it will be woken
> up from its call to wait or wait_timeout. Calls to notify_one are not buffered
> in any way.
This test was relying on the main loop obtaining the lock and entering the
condition variable sleep before the thread was scheduled and got around to
notifying the condition variable. If this non-deterministic behavior was not
upheld, the test would time out since it would obtain the lock (either before or
after the variable were updated) then call `condvar.wait()` *after* the variable
had been updated and the condvar signalled, but without (atomically or even at
all) checking to see if the desired wake precondition was fulfilled. As the
child thread had already run and the wake notification was NOT buffered, there
was nothing to wake the running thread.
There really wasn't any way to salvage the test as originally written, since the
write to `ctx.val` was not in any way linked to the acquire/release of the mutex
so regardless of whether or not the main thread obtained the mutex and checked
the value precondition before calling `condvar.wait()`, the child thread's write
could have happened after the check but before the wait() call. As such, the
test has been rewritten to use `wait_while()` but then also updated to bail in
case of a timeout instead of hanging indefinitely (since neither the `ctest`
runner nor the `cargo test` harness was timing out; `cargo test` would only
report that the test had exceeded 60 seconds but as long as it was not executed
with `cargo test -- -Z --ensure-time` (which is only available under nightly),
the test would not halt.
If this test were *intentionally* written to test the scenario that was timing
out, it should be written deterministically in such a way that the main loop
did not run until after it was guaranteed that the variable had been updated
(i.e. by looping until val became 5 or waiting for an AtomicBool indicating the
update had completed to be set), but I'm not sure what the benefit in that would
be since the docs actually guarantee the opposite behavior (the notified state
is explicitly not cached/buffered).
If we have fish code written with the assumption that condvar notifications
prior to *any* call to `Condvar::wait()` *are* buffered, then that code should
of course be revisited in light of this.
Commit 8a7c3ce (Don't abandon line after writing control sequences, 2024-04-06)
was broken by 29f2da8 (Toggle terminal protocols lazily, 2024-05-16), fix that.
Fixes#10529
