Project/code review requested

[Richardcavell]

Good morning, gentlemen and ladies.

I have written a small project in the C89 dialect of C. I am requesting a sizeable code review - in fact a review of the entire project - for anyone intrepid enough to examine my code.

The code is here: BASICloader/BASICloader.c at master * richardcavell/BASICloader * GitHub

It is 1400 lines of C, and it outputs BASIC source code.

My goals are:

* Coding to a professional standard
* Strict C89 standards compliance
* Correct use of the standard library (and no other library/API)
* Code correctness
* Detection and graceful handling of all possible errors

Specific issues for this project are:

* Wise choice of data types, to accommodate range requirements
* Detection of overflow and other arithmetic problems
* It should be easy to add other target architectures, and easy to see
where the additional code needs to go
* Someone might try to compile this with a compiler or machine that
was created before the C89 standard even existed.

I would be enormously grateful for anyone who could give me feedback. Thanks.

Richard

[jimblumberg]

* Coding to a professional standard

Fail, big time.

1400 lines in a single file, really?

Cryptic variable names, learn to use meaningful variable and function names.

Be careful of mixing typedef with #define, typedef happens at compile time, #define happens at pre-processor time so typedefs are probably not available to #define.

* Strict C89 standards compliance

This is 2017 why are you still stuck in 1990?

* Correct use of the standard library (and no other library/API)

Too much cryptic code for me to even try to see if you're making any mistakes. Did you carefully test each of your functions as you made them to see if they meet your design constraints?

* Code correctness

See above.

* Detection and graceful handling of all possible errors

It doesn't seem like you're very consistent. Look at this snippet:

internal_error(const char *fmt, ...)
{
  va_list ap;

  (void) fprintf(stderr, "Internal error: ");

  va_start(ap, fmt);
  (void) vfprintf(stderr, fmt, ap);
  va_end(ap);

  fprintf(stderr, "\nPlease report this to Richard Cavell\n"
                  "at [email protected]");

Why are you specifically ignoring the return values from the first two print statements by using the cast, but fail to either check the final return value or cast the return value to a void?

IMO, several of your functions have too many parameters, consider using structures to simplify if possible.


Jim

[Hodor]

Things like this confuse me:

static void



check_line_number(line_type *line)

Why are you passing this as a pointer when: a) it is a simple type (currently unsigned short int); and b) you are not modifying the object it's pointing to?

Edit:
I haven't compiled the code, but:

if (*line < old_line || *line + *step > LINE_TYPE_MAX)

Can *line + *step ever be greater than LINE_TYPE_MAX? To me it doesn't seem possible.

[GReaper]

Let's see... The things I checked:
*) Your indentation is almost consistent but too compressed for my taste. That's a personal preference of course, I prefer 4 spaces per level.
*) Don't explicitly ignore return values, that just bloats the code up in my opinion. Reviewers can figure out for themselves whether that function returns something or not.
*) You don't need to explicitly state that the first constant of every enum is zero, that behavior is already standard.
*) Why are you using static everywhere? This isn't a linkable library.