- `libc::setlinebuf` is not available through Rust's libc it appears.
- autocxx fails to generate bindings using `*mut FILE`, instead go through
`void*`
- rust_main needs `parse_util_detect_errors_in_ast`, which is _partially_
ported, instead add FFI interop for C++.
- We need to set the filename if we are sourcing a file
This adopts the new function store, replacing the C++ version.
It also reimplements builtin_function in Rust, as these was too coupled to
the function store to handle in a separate commit.
Historically fish has used the functions `fish_wcstol`, `fish_wcstoi`, and
`fish_wcstoul` (and some long long variants) for most integer conversions.
These have semantics that are deliberately different from the libc
functions, such as consuming trailing whitespace, and disallowing `-` in
unsigned versions.
fish has started to drift away from these semantics; some divergence from
C++ has crept in.
Rename the existing `fish_wcs*` functions in Rust to remove the fish
prefix, to express that they attempt to mirror libc semantics; then
introduce `fish_` wrappers which are ported from C++. Also fix some
miscellaneous bugs which have crept in, such as missing range checks.
Largely routine but for the trampolines in iothread.h and iothread.cpp which
were a real PITA to get correct w/ all their variants.
Integration is complete with all old code ripped out and the tests using the
rust version of the code.
The translation is fairly direct though it adds some duplication, for example
there are multiple "match" statements that mimic function overloading.
Rust has no overloading, and we cannot have generic methods in the Node trait
(due to a Rust limitation, the error is like "cannot be made into an object")
so we include the type name in method names.
Give clients like "indent_visitor_t" a Rust companion ("IndentVisitor")
that takes care of the AST traversal while the AST consumption remains
in C++ for now. In future, "IndentVisitor" should absorb the entirety of
"indent_visitor_t". This pattern requires that "fish_indent" be exposed
includable header to the CXX bridge.
Alternatively, we could define FFI wrappers for recursive AST traversal.
Rust requires we separate the AST visitors for "mut" and "const"
scenarios. Take this opportunity to concretize both visitors:
The only client that requires mutable access is the populator. To match the
structure of the C++ populator which makes heavy use of function overloading,
we need to add a bunch of functions to the trait. Since there is no other
mutable visit, this seems acceptable.
The "const" visitors never use "will_visit_fields_of()" or
"did_visit_fields_of()", so remove them (though this is debatable).
Like in the C++ implementation, the AST nodes themselves are largely defined
via macros. Union fields like "Statement" and "ArgumentOrRedirection"
do currently not use macros but may in future.
This commit also introduces a precedent for a type that is defined in one
CXX bridge and used in another one - "ParseErrorList". To make this work
we need to manually define "ExternType".
There is one annoyance with CXX: functions that take explicit lifetime
parameters require to be marked as unsafe. This makes little sense
because functions that return `&Foo` with implicit lifetime can be
misused the same way on the C++ side.
One notable change is that we cannot directly port "find_block_open_keyword()"
(which is used to compute an error) because it relies on the stack of visited
nodes. We cannot modify a stack of node references while we do the "mut"
walk. Happily, an idiomatic solution is easy: we can tell the AST visitor
to backtrack to the parent node and create the error there.
Since "node_t::accept_base" is no longer a template we don't need the
"node_visitation_t" trampoline anymore.
The added copying at the FFI boundary makes things slower (memcpy dominates
the profile) but it's not unusable, which is good news:
$ hyperfine ./fish.{old,new}" -c 'source ../share/completions/git.fish'"
Benchmark 1: ./fish.old -c 'source ../share/completions/git.fish'
Time (mean ± σ): 195.5 ms ± 2.9 ms [User: 190.1 ms, System: 4.4 ms]
Range (min … max): 193.2 ms … 205.1 ms 15 runs
Benchmark 2: ./fish.new -c 'source ../share/completions/git.fish'
Time (mean ± σ): 677.5 ms ± 62.0 ms [User: 665.4 ms, System: 10.0 ms]
Range (min … max): 611.7 ms … 805.5 ms 10 runs
Summary
'./fish.old -c 'source ../share/completions/git.fish'' ran
3.47 ± 0.32 times faster than './fish.new -c 'source ../share/completions/git.fish''
Leftovers:
- Enum variants are still snakecase; I didn't get around to changing this yet.
- "ast_type_to_string()" still returns a snakecase name. This could be
changed since it's not user visible.
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.
A `block_t` instance is allocated for each live block type in memory when
executing a script or snippet of fish code. While many of the items in a
`block_t` class are specific to a particular type of block, the overhead of
`maybe_t<event_t>` that's unused except in the relatively extremely rare case of
an event block is more significant than the rest, given that 88 out of the 216
bytes of a `block_t` are set aside for this field that is rarely used.
This patch reorders the `block_t` members by order of decreasing alignment,
bringing down the size to 208 bytes, then changes `maybe_t<event_t>` to
`shared_ptr<event_t>` instead of allocating room for the event on the stack.
This brings down the runtime memory size of a `block_t` to 136 bytes for a 37%
reduction in size.
I would like to investigate using inheritance and virtual methods to have a
`block_t` only include the values that actually make sense for the block rather
than always allocating some sort of storage for them and then only sometimes
using it. In addition to further reducing the memory, I think this could also be
a safer and saner approach overall, as it would make it very clear when and
where we can expect each block_type_type_t-dependent member to be present and
hold a value.
Let's hope this doesn't causes build failures for e.g. musl: I just
know it's good on macOS and our Linux CI.
It's been a long time.
One fix this brings, is I discovered we #include assert.h or cassert
in a lot of places. If those ever happen to be in a file that doesn't
include common.h, or we are before common.h gets included, we're
unawaringly working with the system 'assert' macro again, which
may get disabled for debug builds or at least has different
behavior on crash. We undef 'assert' and redefine it in common.h.
Those were all eliminated, except in one catch-22 spot for
maybe.h: it can't include common.h. A fix might be to
make a fish_assert.h that *usually* common.h exports.
Intern'd strings were intended to be "shared" to reduce memory usage but
this optimization doesn't carry its weight. Remove it. No functional
change expected.
We store filenames in function definitions to indicate where the
function comes from. Previously these were intern'd strings. Switch them
to a shared_ptr<wcstring>, intending to remove intern'd strings.
ESCAPE_ALL is not really a helpful name. Also it's the most common flag.
Let's make it the default so we can remove this unhelpful name.
While at it, let's add a default value for the flags argument, which helps
most callers.
The absence of ESCAPE_ALL makes it only escape nonprintable characters
(with some exceptions). We use this for displaying strings in the completion
pager as well as for the human-readable output of "set", "set -S", "bind"
and "functions".
No functional change.
Prior to this commit, setting a universal variable may trigger syncing
against the file which will modify other universal variables. But if we
want to support multiple environments we need the parser to decide when to
sync uvars. Shift the decision of when to sync to the parser itself. When a
universal variable is modified, now we just set a flag and it's up to the
(main) parser when to pick it up. This is hopefully just a refactoring with
no user-visible changes.
These macros were historically used only in internal error messages which
should never happen! Now we are able to enforce they never happen at
compile time so we can remove them.
No functional change here.
If we ever need any of these... they're in this commit:
fish_wcswidth_visible()
status_cmd_opts_t::feature_name
completion_t::is_naturally_less_than()
parser_t::set_empty_var_and_fire()
parser_t::get_block_desc()
parser_keywords_skip_arguments()
parser_keywords_is_block()
job_t::has_internal_proc()
fish_wcswidth_visible()
We need special handling when reporting backtraces for commands run
during startup, i.e. config.fish. Previously we had a global variable;
make it local to the parser to eliminate a global.
No functional change here.
Cancellation groups were meant to reflect the following idea: if you ran a
simple block:
begin
cmd1
cmd2
end
then under job control, cmd1 and cmd2 would get separate groups; however if
either exits due to SIGINT or SIGQUIT we also want to propagate that to the
outer block. So the outermost block and its interior jobs would share a
cancellation group. However this is more complex than necessary; it's
sufficient for the execution context to just store an int internally.
This ought not to affect anything user-visible.
This disables job control inside command substitutions. Prior to this
change, a cmdsub might get its own process group. This caused it to fail
to cancel loops properly. For example:
while true ; echo (sleep 5) ; end
could not be control-C cancelled, because the signal would go to sleep,
and so the loop would continue on. The simplest way to fix this is to
match other shells and not use job control in cmdsubs.
Related is #1362
is_block is a field which supports 'status is-block', and also controls
whether notifications get posted. However there is no reason to store
this as a distinct field since it is trivially computed from the block
list. Stop storing it. No functional changes in this commit.
