-
Notifications
You must be signed in to change notification settings - Fork 254
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Update the Rust's brainfuck implementation (#325)
* Update the Rust's brainfuck implementation More idiomatic code, and formatting with `rustfmt`. The most important change was in the `Tape` impl: * `mov()` reallocs only once per call; * `get()` and `inc()` uses unchecked indexing because the check is already done in `mov()` * Pass String as reference * Use more specific enum variants Because of the strong typing, the `mov()` was doing many lossy casts which the C and C++ versions don't; the D version uses specific variants too. Besides, the value was always -1 or 1, so it is more idiomatic this way.
- Loading branch information
Showing
1 changed file
with
96 additions
and
43 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7e2fc03
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit introduces a memory safety issue that wasn't present before. Consider the program
<.
:With the old version,
<
would overflow the tape position tousize::MAX
, then attempt to resize the tape in a loop until the new size isusize::MAX
. This is actually a non-terminating loop because the tape length is bounded byusize
too, so it can never be greater than the tape position. This is bad, but at least it's memory-safe.With the new version up to 856b1a1 (HEAD as of writing) ,
<
overflows the tape position tousize::MAX
without any subsequent resize/bounds check, then.
can be used to print the contents of arbitrary memory beyond the tape array.This issue is also present with e.g. the current C++ implementation which checks nothing, and I would argue it is an incorrect implementation too. Since the goal of Rust is to prevent this kind of issue, removing bounds checks is pretty misleading from a benchmarking perspective. (not saying it's intentional here, it seems like either a legitimate oversight or optimizing for the test input, but in the latter case, why bother mentioning safety in recent commits at all?)
7e2fc03
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you! I've looked into the original implementation (mirror in https://gitlab.com/nuald-grp/bf-2), and they have the range check. As the check could be a bottleneck, some tests (including Rust and C++) may have the unfair advantage. I've created a ticket #473 and will try to address it in my spare time. It could be a little bit a challenge for functional programming languages as it introduces the side effect, but I guess it's unavoidable as exceptional situations should be properly handled.
7e2fc03
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the
debug
build an underflow will trigger a panic, that the right thing to do in this case. However, because the reference impl (C++) doesn't do bound checks, I didn't do that inrelease
builds, or it would be unfair...If the C++ impl change that, the Rust version can easily check using
usize::checked_sub
.