Skip to main content
Improved formatting. Reworded a few sentences plus other minor cleanups.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89

C style:

C style

Well, the first thing I have to say is that your code is very C-ishlike. The The first action here would be to move all those loose functions and globals into two two C++ classes, Lexer and Parser.

int i;
for (i = 0; i < prog.size(); ++i) // poor practice
----
for (int i = 0; i < prog.size(); ++i) // much better

And yet another old school C habit is to use the /**/ (multi-line comments) everywhere. Why Why not use the much more practical // for single liners? The single line comment is even valid in C now.

using namespace std:

using namespace std

Possible bug?

Possible bug?

bool rdo_is_reserved(string tok) {
  int i;
  for (i = 0; i < 9;i++) {
    if (tok == reserved[i])
      return true;
    else
      return false;
  }
  return false;
}
bool rdo_is_reserved(string tok) {
  int i;
  for (i = 0; i < 9;i++) {
    if (tok == reserved[i])
      return true;
    else
      return false;
  }
  return false;
}

This function is iterating from 0 to 98, however, the reserved vectorarray has 14 elements. Is this intentional or a bug? Did you mean reserved.size()? But this function is useless anyway, since since the standard libraryStandard Library provides std::find(), which you should be using insteadis meant for this kind of linear searches and conveys intent much more clearly.

Commented lines leftover:

Commented lines leftover

Large functions:

Large functions

The lex() function is a massive beast. I wouldn't want to be the poor programmer tasked with the job of modifying it or fixing a bug caused by it. This This function needs an urgent refactoring. It should be broken down into several several helper methods.

Calling exit():

Calling exit()

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup or error message. It It should instead throw an exception. This is C++ and we have much more modern ways ways of handling errors, plus with an exception the caller has the chance of recovering from the error if possible.

goto and the lost label:

goto and the lost label

You have a label inside the parse() function, TOP:, which I would expect be paired with a goto statement somewhere. However, I couldn't find a single goto inside the function. So what is that label even doing there?

Since you never did CS school, you might not be aware of the status of goto. Lets.. Lets just say that it is quite controversial. I I for one really don't think goto still havehas a place in modern C++, like I said before before, the language has exceptions and automatic resource management, so goto is pretty arcane pretty outdated.

couts:

couts

Your code is also littered with cout calls, most are commented out. You shouldn't be using it directly inside functions like you did. Most users won't want the verbose output on every execution of the program. If you need them for debugging, a better approach is to wrap those calls into a macro that can be disables atdisabled at compile time, e.g.:

Or use a better, more mature, logger APIlog library. Do a search and you will find a ton of good options.

C style:

Well, the first thing I have to say is that your code is very C-ish. The first action here would be to move all those loose functions into two C++ classes, Lexer and Parser.

int i;
for(i = 0; i < prog.size(); ++i) // poor practice
----
for(int i = 0; i < prog.size(); ++i) // much better

And yet another C habit is to use the /**/ (multi-line comments) everywhere. Why not use the much more practical // for single liners?

using namespace std:

Possible bug?

bool rdo_is_reserved(string tok) {
  int i;
  for (i = 0; i < 9;i++) {
    if (tok == reserved[i])
      return true;
    else
      return false;
  }
  return false;
}

This function is iterating from 0 to 9, however, the reserved vector has 14 elements. Is this intentional or a bug? Did you mean reserved.size()? But this function is useless anyway, since the standard library provides std::find(), which you should be using instead.

Commented lines leftover:

Large functions:

The lex() function is a massive beast. I wouldn't want to be the poor programmer tasked with the job of modifying it or fixing a bug caused by it. This function needs an urgent refactoring. It should be broken down into several helper methods.

Calling exit():

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup or error message. It should instead throw an exception. This is C++ and we have much more modern ways of handling errors.

goto and the lost label:

You have a label inside the parse() function, TOP:, which I would expect be paired with a goto statement. However, I couldn't find a single goto inside the function. So what is that label even doing there?

Since you never did CS school, you might not be aware of the status of goto. Lets just say that it is quite controversial. I for one really don't think goto still have a place in modern C++, like I said before, the language has exceptions and automatic resource management, so goto is pretty arcane.

couts:

Your code is also littered with cout calls, most are commented out. You shouldn't be using it directly inside functions like you did. Most users won't want the verbose output on every execution of the program. If you need them for debugging, a better approach is to wrap those calls into a macro that can be disables at compile time, e.g.:

Or use a better, more mature, logger API. Do a search and you will find a ton of good options.

C style

Well, the first thing I have to say is that your code is very C-like. The first action here would be to move all those loose functions and globals into two C++ classes, Lexer and Parser.

int i;
for (i = 0; i < prog.size(); ++i) // poor practice
----
for (int i = 0; i < prog.size(); ++i) // much better

And yet another old school C habit is to use the /**/ (multi-line comments) everywhere. Why not use the much more practical // for single liners? The single line comment is even valid in C now.

using namespace std

Possible bug?

bool rdo_is_reserved(string tok) {
  int i;
  for (i = 0; i < 9;i++) {
    if (tok == reserved[i])
      return true;
    else
      return false;
  }
  return false;
}

This function is iterating from 0 to 8, however, the reserved array has 14 elements. Is this intentional or a bug? But this function is useless anyway, since the Standard Library provides std::find(), which is meant for this kind of linear searches and conveys intent much more clearly.

Commented lines leftover

Large functions

The lex() function is a massive beast. I wouldn't want to be the poor programmer tasked with the job of modifying it or fixing a bug caused by it. This function needs an urgent refactoring. It should be broken down into several helper methods.

Calling exit()

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup or error message. It should instead throw an exception. This is C++ and we have much more modern ways of handling errors, plus with an exception the caller has the chance of recovering from the error if possible.

goto and the lost label

You have a label inside the parse() function, TOP:, which I would expect be paired with a goto statement somewhere. However, I couldn't find a single goto inside the function. So what is that label even doing there?

Since you never did CS school, you might not be aware of the status of goto... Lets just say that it is quite controversial. I for one really don't think goto still has a place in modern C++, like I said before, the language has exceptions and automatic resource management, so goto is pretty outdated.

couts

Your code is also littered with cout calls, most are commented out. You shouldn't be using it directly inside functions like you did. Most users won't want the verbose output on every execution of the program. If you need them for debugging, a better approach is to wrap those calls into a macro that can be disabled at compile time, e.g.:

Or use a better, more mature, log library. Do a search and you will find a ton of good options.

Small fix.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup or error message. It should instead throw an exception. This is C++ and we have much more modern ways of handling errors.

You have a label inside the parse() function, TOP:, which I would expect be paired with a goto statement. However, I couldn't find a single goto inside the function. So So what is that label even doing there?

#define DEBUG_LOG(msg) do { std::cout << msg << std::endend; } while(0)

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup. It should instead throw an exception. This is C++ and we have much more modern ways of handling errors.

You have a label inside the parse() function, TOP:, which I would expect be paired with a goto statement. However, I couldn't find a single goto inside the function. So what is that label even doing there?

#define DEBUG_LOG(msg) std::cout << msg << std::end

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup or error message. It should instead throw an exception. This is C++ and we have much more modern ways of handling errors.

You have a label inside the parse() function, TOP:, which I would expect be paired with a goto statement. However, I couldn't find a single goto inside the function. So what is that label even doing there?

#define DEBUG_LOG(msg) do { std::cout << msg << std::end; } while(0)
Small fix.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89

Well, the first thing I have to say is that your code is very C-ish. The first thing to doaction here iswould be to move all those loose functions into two C++ classes, Lexer and Parser.

Counters in for loops, especially, should be declared inside the loop:

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup. It should instead throw an exception. This is C++ and we have much more modern ways of handling errors.

You have a label inside the parse() function, TOP:, which I presume would expect be paired with a goto statement. However, I couldn't find a single goto inside the function. So what is that label even doing there?

Well, the first I have to say is that your code is very C-ish. The first thing to do here is to move all those loose functions into two C++ classes, Lexer and Parser.

Counters in for loops, especially should be declared inside the loop:

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? It should instead throw an exception. This is C++ and we have much more modern ways of handling errors.

You have a label inside the parse() function, TOP:, which I presume would be paired with a goto statement. However, I couldn't find a single goto inside the function. So what is that label even doing there?

Well, the first thing I have to say is that your code is very C-ish. The first action here would be to move all those loose functions into two C++ classes, Lexer and Parser.

Counters in for loops, especially, should be declared inside the loop:

lex() also calls exit(1) in its body. This is a bit harsh, don't you think? Terminating the program like this, without cleanup. It should instead throw an exception. This is C++ and we have much more modern ways of handling errors.

You have a label inside the parse() function, TOP:, which I would expect be paired with a goto statement. However, I couldn't find a single goto inside the function. So what is that label even doing there?

Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading