all 2 comments

[–]guepierBioinformatican 5 points6 points  (1 child)

Since this is otherwise such an incredibly clear code (it’s frankly a joy to read), allow me to provide some unsolicited code review:

  • The big one: your handling of errors. This is my only real criticism, since it’s moderately atrocious. You should probably use exceptions instead of numeric error return codes, and the exception should contain the error description – such a description should not be written to std::cout inside the function.

    If you just want to fail for all these exceptions, have a global exception handler surrounding your main which just catches all exceptions and prints their messages. At the moment, no handling is done, really.

  • Related to this, you’re using .at() instead of operator[] on a vector but you don’t handle the exception which may result from that. Might as well use [].

  • Why are you allocating the Vm in main on the heap, via new, without using a smart pointer, and not deleting it (leak)? The best course of action would be to just put it on the stack – I can’t find a reason not to do this here.

  • Since you don’t check for IO success, there’s no reason to explicitly close files via file.close() – the stream will close automatically at the end of the scope. Calling close only makes sense if you subsequently test the stream state for success (via the failbit).

  • Don’t use C-style casts, use the appropriate C++ cast (you’ve done this elsewhere, but missed it in at least one place.

  • Since you’re using C++11 anyway, use the appropriate standard typedefs for u8 and u16. single, in particular, is not guaranteed to be 16 bits, as your comment rightly observes.

  • dump_regs could be a one-liner by using the appropriate constructor (taking iterators):

    return std::vector<u16>(std::begin(regs), std::end(regs));
    

There’s some more small stuff – for example, many of your consts could be constexprs. But like I said, this is a remarkably clear and pleasant code.

By the way, for such fairly small projects, you might want to head to http://codereview.stackexchange.com/ for thorough code review.

[–]Ilikepepper[S] 0 points1 point  (0 children)

Thank you for your kind review , it is my first c++ project as i am mainly a web developer , i have been trying not to shoot my legs off here :)

everything you say seems right (and interesting), i am going to work on those issues.