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 ...


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 ...


43

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 *...


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 ...


18

I was asked by @Timtech to join + post an improvement. Using arrays is a lot easier to understand, and there were so many optimizations that I decided to completely re-write the code. I'm sure it could be improved by other advanced programmers like me, as I'm using several long expressions here... note that L1 represents list #1 (2nd + 1 on the calculator) ...


17

I second the recommendation to invent a grammar and use a real parser. A recursive descent parser is very easy to write, and a great place to start. You might also have a look at a PEG (Parsing Expression Grammar). It's almost as easy to use a PEG as it is to write a recursive descent compiler, and a PEG can parse more grammars than can a recursive ...


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

Beyond the bad names that you are already aware of, I see a few things that could be improved. str1 and str2 are bad variable names as well. They should be str and chr respectively. Those names would properly represent the data and make this code much more understandable. It would be instantly clear to anyone looking at the code that you're looping through ...


14

Obviously, if you have any questions just ask. This is my first "big" project in OCaml, but I'd rather you didn't sugarcoat criticism. It looks like a very fun project, congratulations! :) I have a few comments, which all concerns the style of writing. This is an important topic, because a good style eases maintainability. The name of constructors: all ...


13

Loki Astari already covered a number of good points, which I will not repeat. Algorithms and data structures The standard library contains a number of ready-made algorithms and data structures that can make your code easier to read and understand. For example, you have these lines: opp_count = 0; // line 393, new lines removed for brevity for (i = 0; i &...


12

Common beginner mistakes Stop doing this: using namespace std; See Why is “using namespace std;” considered bad practice? Namespaces All your functions seem to have the prefix rdo_ bool rdo_ws(char c) char rdo_expr_item_type(char c) string rdo_opp_to_string(char opp) bool rdo_is_num(string is_num) void rdo_count_opp(bool to_count_or_not) string ...


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 ...


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 ...


11

Why do you allow each cell of the tape to hold numbers from -1 to 128? seems like an odd range. in move_backward() why do you allow the tape to reach position -1? in move_forward() why do you allow the tape's position to be beyond the end of the tape? In general you should be using exclusive comparisons (without the =) as you'll make fewer mistakes.


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 ...


10

It is a beginning. But currently your code is just a thin wrapper around Python function calls. And of course, there is a security problem with "eval", because someone could format your harddisk with the right line, if you execute scripts from untrusted sources. Maybe you should invent a nice syntax for your language. An easy method to write a parser for it,...


10

As the classic "Stop Writing Classes" puts it: the signature of "this shouldn't be a class" is that it has two methods, one of which is __init__ Virtually all of your classes fall foul of this; just because you can use OOP, doesn't mean you always should. Looking at the use of the classes in the code, this was a big red flag: code_input = GetCodeInput(...


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

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

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

bcode = [] stack = [] regs = [] sp = 0 bcd = [] Global variables like this are frowned upon. To be pythonic you should really put them a in a class or something. ''' Instructions ''' OP_EOP = 0 OP_EOI = 1 OP_PUSH = 2 OP_PRINT = 3 def load_program(f2o): f2o? What in the world is that? f = open(f2o, "r") f2 = f.read() My recollection of RPython ...


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 line here: return EXIT_SUCCESS Is equivalent to this: return 0; Which is automatically inserted by the compiler if it isn't found. In short, return EXIT_SUCCESS can be removed. In addition, a few of your error messages don't include a newline at the end, as seen in this line here, and two other places: std::cerr << "compilation terminated."; ...


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

I really don't like that you detect which version of _GetchX to use via an ImportError - that isn't obvious to me at all. I also don't like that you keep importing things locally. I think you can solve this like so: import platform system = platform.system() import sys if system == "Windows": import msvcrt class _Getch: """Gets a ...


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 ...


8

Expectations Lisp is well-known for it's REPL, so I'd expect to see at least the following 3 functions in any Lisp interpreter: read, evaluate and print. read takes a string and returns a form (a number, string, symbol, list, and so on). For example: read("(+ 4 5)") should return a list that contains the symbol + and the numbers 4 and 5. evaluate takes a ...


8

All of your if char == '>': ptr += 1 and similar checks should use elif after the first check. By using if for all of the checks, you're forcing them all to run, even once you've found a match. This is wasteful because the checks are necessarily exclusive of each other. Once one check is true, none of the other checks can be. For example, you should have: ...


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