50

Congratulations on your first large project. I'm not sure whether this review has grown a little bit overboard, as it is now both a review as well as a mini tutorial. Either way: What the char? Charmander-char Char cha Charmander Char. Char? Charmander! Is it confusing in general? Char! I mean, yes. Mostly due to the names of your functions. As ...


49

Simplifications Thanks to implementing some analysis of while loops in my Brainduck project (which I worked on thanks to reviewing "Hello, Brainfuck"), I managed to find a bunch of loops that were always performed x times. By expanding these loops (copy-paste their content x times and remove the loop) several simplifications are possible. Boolean peek-a-...


44

The comment ; *argv should be ; argv, since you are not yet dereferencing the pointer. After a cmp instruction, you should prefer je over jz, since it is nicer to the human reader. Oh, the old times, where you had to tell the assembler to jmp short because it couldn't figure it out on its own. :) In the run the BF program section, I would have changed esi ...


44

This is a pretty reasonable start on a simple interpreter. Edward's suggestions are all good; a few additional suggestions: interpret("+++++++++++++[->.... Please break up that long line. C allows you to break up literal strings "like " "this." void goToLoopEnd(char** ip) { ... void goToLoopStart(char** ip) { ... If you wrote these instead as char *...


40

Congratulations. This was pretty readable code, by brainfuck standards. I was able to follow it. By initializing your max cell to 256, you've made your code portable to work on non-wrapping brainfuck interpreters as well. If you just wanted it to work on a wrapping brainfuck interpreter (i.e., one that works modulo 256), you could have left the cell set to ...


39

You should "initialize 100" after the "Tape meanings". I consider it to be part of the actual code and the explanation to be a kind of "header" explaining the code. I agree that commenting "what" becomes more important in bf, but you should still explain "why" every now and then. The reasoning behind why you build the "Fizz" and "Buzz" strings the way you ...


33

No, that's unreadable code and therefore not idiomatic. The typical way to generate large numbers is to use multiplication. "Hello, World!" translates to ASCII values < 72, 101, 108, 108, 111, 44, 32, 87, 111, 114, 108, 100, 33 > … which can be thought of as 10 * < 7, 10, 11, 11, 11, 4, 3, 9, 11, 11, 11, 10, 3 > + < 2, 1, -2, -2, ...


33

Brainfuck Multiplication Elaborating a bit more about 200_success' multiplication, and the "shorter, less readable version, using similar ideas, but with more cell reuse": The ASCII values of what you want to write is, as 200_success mentioned: 72 101 108 108 111 44 32 87 111 114 108 100 33 So these are the numbers we want to generate. Let's start by ...


26

Here are some things that may help you improve your program. In all, it seems to be nice, straightforward code that does what it needs to do. Good start! Use only required #includes The code has #include <stdbool.h> but doesn't use booleans. It also appears that nothing from <stdlib.h> is used either. Only include files that are actually ...


23

Additional C#6.0 Modifications How C#6.0 would you like it to be? You still have a few places you can use more C#6.0: I'm going to work bottom-to-top in your post: Usually, I don't support using expression-bodied members on void methods, but this class would benefit from a few of them: public class ExecutionContext { public ExecutionContext(int ...


16

Good job on getting it to work! I've used a string reversal program to check your interpreter and it works well. However, it also uses ~36MB of memory, which is too much. A tape goes both directions equally fast Forward, rewind. The basic operations for a tape. Whether it's VHS, a cassette, or a LTO-8, they all work the same: accessing the next and ...


15

Move the memory into it's own class. The concept of the tape which the BF program is operating can be cleanly made its own class with a limited interface. Make private or eliminate interface cruft: perform, getMemoryIndex, reset, addCommands, setCommands, step. For a BF interpreter, it really only make sense to set a particular program and then run it. ...


15

Analysis I analyzed your code with my Brainduck and I found a couple of improvements. First up, let's see what Brainduck says. I will not post the full output here, just the parts that I found the most interesting. First of all, at runtime your program performs approximately 1.47 MILLION commands. This is quite much. While loop - 49 to 58 My analysis ...


12

Looks great! Just a few (small) suggestions: Style It really is quite readable and straightforward, but breaking out a few functions certainly wouldn't hurt. Even though you're not using using namespace std; (woo!), the variable name stack still makes me a bit uncomfortable (then again, I can't think of a name for it that wouldn't end up being gross). If ...


12

This looks really good over all. I have pretty much no knowledege of brainfuck, but it was still easy to understand the code, and nothing jumps out at me as glaringly wrong. There are however a few (mostly) minor issues. run flag and compiler optimizations volatile is a widely misused variable modifier, but you acutally have one of the text book examples ...


11

Dayum, there's some static around here. Why not make it an instance class? What you have here is a workflow, where methods have a specific order. This is clearly seen in your Main method. The workflow usually have a state, and static stuff is supposed to be stateless. So I really think an instance class would be good! Doing this would also mean you wouldn'...


10

I think this would be a more efficient way of doing it (at least for the printable ASCII characters): ++++[->++++++++<] Use cell 0 for loop and cell 1 for ASCII codes ++++[->>++++++++<<]>>> Use cell 2 for the space +++>++> Use cells 3 and 4 for the ...


10

Use case instead of == and guards everywhere: prevBracketIndex :: Int -> Int -> Array Int Char -> Int prevBracketIndex i depth cs = case cs ! i of '[' -> if (depth - 1) == 0 then i else prevBracketIndex (i - 1) (depth - 1) cs ']' -> prevBracketIndex (i - 1) (depth + 1) cs _ -> prevBracketIndex (i - 1) depth cs Use State and ...


9

Deficiencies? I don't really see any. Improvements? Maybe :) You don't handle at all the return values of putchar, getchar and fflush. The Wikipedia article has some hints about how different implementations handle an EOF from the user input. Yours works as well, but is this really what you want? In build_jump_table, the switch used to check whether *c is a ...


9

The multiplication table is missing formatting. I realise that, and will add it some time later, if I can figure out how to compare two values. Comparing two values isn't that complicated, once you know how to do it. It is however extremely inefficient if you do it over and over again. Your best bet would be to hook into the printing of the number and print ...


9

Portability Your first loop immediately subtracts 4 from a cell whose value is 0. It seems like your program is designed to run using the wraparound dialect of Brainfuck, so it would be a good idea to note that in a comment. On the other hand, writing a Fibonacci sequence generator using the 8-bit dialect isn't that useful, because you would overflow ...


9

Use NULL for null pointers NULL conveys your intention to use a pointer better than 0: if (file == NULL) { ... } As a bonus, in C the macro NULL typically expands to something like (void*)0; if you have warnings for potentially unsafe type conversions enabled (-Wconversion for GCC), the compiler will notify you, if you try to assign 0 to a pointer or NULL ...


8

Usability How the f.[< do I use this interpreter when it doesn't come with a main() function? Here's the simplest implementation I came up with, using java.nio.file.*: public static void main(String[] args) throws IOException { String code = new String(Files.readAllBytes(Paths.get(args[0]))); // TODO: Implement InputStreamToByteIteratorAdaptor ...


8

This being Python, it should be relatively easy to present the illusion of an infinite tape, at least in the positive direction. I don't see reason that cell_amount has to be specified, and the user shouldn't have to worry about such details. Your input and output routines are wrong: The . instruction should print one character, interpreting the cell ...


8

My biggest complaint on readability would be your findmatch() function. There are too many non-descriptive variables (i.e. i, j, p, etc). Changing the names to be more descriptive, or adding some comments to elaborate on what you're trying to do would help a lot.


8

You're correct that nesting a case in an elsif ladder is a bit clunky here, but the two things that jump out at me as making the code difficult to comprehend are: 1) we have to know everything that's going on with the Regexp passed to scan() in order to figure out the intent of the if clauses, and 2) the variable string is not a String, but an Array. You ...


8

First of all, let's not beat around the bush. Clearly your filenames should be Brainfuck.hpp and Brainfuck.cpp. Let's not be shy. Now for the real reviews: Input std::string currentLine = " "; // do not start empty while (currentLine.length() != 0) // escape input by an empty line { std::getline(std::cin, currentLine); userInput.push_back(...


8

Just a few very minor points... There's a couple places where you do this sort of thing: string brainfuck = string.Empty; using (StreamReader sr = new StreamReader(filename)) { brainfuck = sr.ReadToEnd(); } Which could be more easily written as: var brainfuck = File.ReadAllText(filename); StringBuilder's append methods return the StringBuilder so ...


8

if ((OptimizationLevel & OptimizationLevel.Level1) >= OptimizationLevel.Level1) { if (lastSymbol != symbol && lastSymbol != TokenSymbol.None && (lastSymbol == TokenSymbol.Decrement || lastSymbol == TokenSymbol.Increment || lastSymbol == TokenSymbol.MoveLeft || lastSymbol == TokenSymbol.MoveRight)) { Lines.AddRange(...


8

Bugs Your program doesn't work with nested loops. If you have + jump into the loop [ this is the first loop [ this is the second loop (1) - decrease current value to get out of loop ] + we increment our current value to get back to the start ] whoops, we go to (1) You need to remember the position of the ...


Only top voted, non community-wiki answers of a minimum length are eligible