5
\$\begingroup\$

I made simple scripting language (interpreter) in C and now, I want to make my code better.

Here is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define TRUE (1 == 1)
#define FALSE (1 == 0)

#define FUNCTION_NAME_COUNT 2
#define FUNCTION_ARG_FUNCTION_NAME_COUNT 1
#define MAX_FUNCTION_NAME_LENGTH 247
#define MAX_FUNCTION_ARG_LENGTH 127

#define MAX_STRING_LENGTH 2048

char *itoa(int value, char *str, int base)
{
    char *output;
    char *ptr;
    char *low;
    if (base < 2 | base > 36)
    {
        *str = '\0';
        return str;
    }
    output = ptr = str;
    if (value < 0 & base == 10)
    {
        *ptr++ = '-';
    }
    low = ptr;
    do
    {
        *ptr++ = "zyxwvutsrqponmlkjihgfedcba9876543210123456789abcdefghijklmnopqrstuvwxyz"[35 + value % base];
        value /= base;
    }
    while (value);
    *ptr-- = '\0';
    while (low < ptr)
    {
        char tmp = *low;
        *low++ = *ptr;
        *ptr-- = tmp;
    }
    return output;
}

char *strr(char *str, char *output, int x, int y)
{
    int i;
    for (i = x + 1; i < y; i++)
    {
        output[i - (x + 1)] = str[i];
    }
    return output;
}

char *equstr(char *str, char *output)
{
    int i = 0;
    while (str[i])
    {
        output[i] = str[i];
        i++;
    }
    return output;
}

void clear_str(char *str)
{
    int i = 0;
    while (str[i])
    {
        str[i] = '\0';
        i++;
    }
}

int is_string(char *str)
{
    if (str[0] == '"' & str[strlen(str) - 1] == '"')
    {
        return TRUE;
    }
    else
    {
        return FALSE;
    }
}

int is_sint(char *str)
{
    int i = 0;
    while (str[i])
    {
        if (str[i] >= '0' & str[i] <= '9')
        {
            return TRUE;
        }
        else
        {
            return FALSE;
            break;
        }
    }
}

void lang_prints(char *s)
{
    char output[MAX_STRING_LENGTH] = "\0";
    if (is_string(s))
    {
        strr(s, output, 0, strlen(s) - 1);
        printf("%s\n", output);
    }
}

void lang_printi(int i)
{
    printf("%d\n", i);
}

char *lang_arg_lens(char *s)
{
    char *output = malloc(MAX_STRING_LENGTH);
    if (is_string(s))
    {
        itoa(strlen(s) - 2, output, 10);
        return output;
    }
}

int main()
{
    int i, j;
    char function_names[FUNCTION_NAME_COUNT][MAX_FUNCTION_NAME_LENGTH] = {
        "prints",
        "printi"
    };
    char function_arg_function_names[FUNCTION_ARG_FUNCTION_NAME_COUNT][MAX_FUNCTION_NAME_LENGTH] = {
        "lens"
    };
    char input[MAX_STRING_LENGTH];
    char function_name[MAX_FUNCTION_NAME_LENGTH] = "\0";
    char function_arg[MAX_FUNCTION_ARG_LENGTH] = "\0";
    char tmp_function_arg[MAX_FUNCTION_ARG_LENGTH] = "\0";
    char function_arg_function_name[MAX_FUNCTION_NAME_LENGTH] = "\0";
    char old_function_arg[MAX_FUNCTION_ARG_LENGTH] = "\0";
    printf("> ");
    scanf("%s", input);
    for (i = 0; i < FUNCTION_NAME_COUNT; i++)
    {
        clear_str(function_name);
        strr(input, function_name, -1, strlen(function_names[i]));
        strr(input, function_arg, strlen(function_names[i]), strlen(input) - 1);
        if (strcmp(function_name, function_names[i]) == FALSE)
        {
            if (input[strlen(function_name)] == '(' & input[strlen(function_name) + strlen(function_arg) + 1] == ')')
            {
                for (j = 0; j < FUNCTION_ARG_FUNCTION_NAME_COUNT; j++)
                {
                    if (strcmp(old_function_arg, "\0") == FALSE)
                    {
                        equstr(function_arg, old_function_arg);
                    }
                    clear_str(function_arg);
                    clear_str(function_arg_function_name);
                    strr(function_arg, function_arg_function_name, -1, strlen(function_arg_function_names[j]));
                    strr(old_function_arg, function_arg, strlen(function_arg_function_names[j]), strlen(old_function_arg) - 1);
                    strr(old_function_arg, function_arg_function_name, -1, strlen(function_arg_function_names[j]));
                    if (strcmp(function_arg_function_name, function_arg_function_names[j]) == FALSE)
                    {
                        if (old_function_arg[strlen(function_arg_function_name)] == '(' & old_function_arg[strlen(function_arg_function_name) + strlen(function_arg) + 1] == ')')
                        {
                            switch (j)
                            {
                                case 0:
                                    equstr(function_arg, tmp_function_arg);
                                    clear_str(function_arg);
                                    equstr(lang_arg_lens(tmp_function_arg), function_arg);
                                    break;
                            }
                        }
                    }
                    else
                    {
                        clear_str(function_arg);
                        equstr(old_function_arg, function_arg);
                    }
                }
                switch (i)
                {
                    case 0:
                        lang_prints(function_arg);
                        break;
                    case 1:
                        if (is_sint(function_arg))
                        {
                            lang_printi(atoi(function_arg));
                        }
                        break;
                }
            }
        }
    }
}

Here is the GitHub repository of my scripting language.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Curious, why #define TRUE (1 == 1) instead of true from <stdbool.h>? \$\endgroup\$ – chux - Reinstate Monica Jun 23 at 18:30
  • 1
    \$\begingroup\$ @chux-ReinstateMonica Well, the latter is tied to a true boolean type. Whether that is good (proper type), bad (ABI), or doesn't matter... \$\endgroup\$ – Deduplicator Jun 23 at 18:52
  • 8
    \$\begingroup\$ A sample of the language being parsed would greatly improve this question. \$\endgroup\$ – Edward Jun 23 at 18:54
  • \$\begingroup\$ It would be helpful if you mention which C standard is targeted by your code. This matters stuff like stdbool.h. \$\endgroup\$ – mika Jun 24 at 12:21
5
\$\begingroup\$

Potential bug

str[strlen(str) - 1] is undefined behavior when str[0] == 0. Instead use a logical AND.

// if (str[0] == '"' & str[strlen(str) - 1] == '"') 
if (str[0] == '"' && str[strlen(str) - 1] == '"')

Note is_string("\"") returns true.

Potential bug

int is_sint(char *str) has no return value when str[0] == 0.

Code is strange as it has a while loop but never iterates a second time.

is_sint("-123") returns false.

Design asymmetry

if (value < 0 & base == 10) makes for base 10 a special case. Suggest if (value < 0) instead and then make a companion uitoa(unsigned value, char *str, int base) for a sign-less version.

Consider const and restrict

Both const (source does not changed) and restrict (referenced data only affected via this pointer) can make for faster code and wider application.

Example:

// char *equstr(char *str, char *output)
char *equstr(const char * restrict str, char * restrict output)

Use stdbool.h

Rather than make up TRUE/FASLE, use bool, true, false.

Tighten up array initializations

\0 not needed.

// char output[MAX_STRING_LENGTH] = "\0";
char output[MAX_STRING_LENGTH] = "";

Avoid buffer overflow

scanf("%s", input); is worse than gets(). Use a width or research fgets().

Use size

char *itoa(int value, char *str, int base) belongs to the era when memory was expensive. Make more robust and safe string functions and pass in a size to avoid buffer overrun.

I'd re-order with destination parameters on the left and use another name as itoa() is likely to collide with other code bases.

// char *itoa(int value, char *str, int base)
char *my_itoa(size_t str_size, char *str, int value, int base)

Pedantic: Large string support

strr(char *str, char *output, int x, int y) relies on the length of a string not exceeding INT_MAX. The length of a string may approach the potentially much larger SIZE_MAX.

Does not apply with this main(), but consider code re-use.

| vs. ||

if (base < 2 || base > 36) is more idiomatic than if (base < 2 | base > 36), yet makes no functional difference here.

Perhaps a shorter 0-9a-z string?

Include the usual suspects.

// Enough room INT_MIN as a binary string.
#define MY_ITOA_BUF_SIZE (sizeof(int) * CHAR_BIT + 2)
#define MY_ITOA_BASE_MIN 2
#define MY_ITOA_BASE_MAX 36

// Negative values result in a string beginning with a '-' for all bases
//
char* my_itoa(size_t n, char *s, int i, int base) {
  char buf[MY_ITOA_BUF_SIZE];
  char *p = &buf[sizeof buf - 1];
  assert(base >= MY_ITOA_BASE_MIN && base <= MY_ITOA_BASE_MAX);

  *p = '\0';
  int value = i < 0 ? i : -i;  // negative absolute value
  do {
    div_t qr = div(value, base);
    *(--p) = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"[-qr.rem];
    value = qr.quot;
  } while (value);

  if (i < 0) {
    *(--p) = '-';
  }

  size_t n_used = (size_t) (&buf[sizeof buf] - p);
  // If not enough room
  if (n_used > n) {
    if (n > 0) {
      *s = '\0';
    }
    return NULL;
  }
  return memcpy(s, p, n_used);
}
\$\endgroup\$
1
  • \$\begingroup\$ Perhaps you could mention sscanf and sscanf_s (C11 and up) explicitly in your point about scanf? \$\endgroup\$ – Jan Dorniak Jun 24 at 16:18
3
\$\begingroup\$

Boolean type

If you really want / need a boolean type, use <stdbool.h>. If you don't, just use an int as much prior code enshrines.
But for the love of all, don't define your own TRUE. Remember that only null and zero are falsey, all else being truthy. Defining a constant invites comparison against it, which violates this convention imbued into the language itself.
Defining FALSE while also worrying at least won't lead to compiling code which is logically wrong. But that is all it has going for itself.

Boolean logic

If you have a boolean value to return, return it directly, don't branch on it and return literals. Unless you are paid by LoC, it's a pointless obfuscation.

Unless you have a good reason, use boolean operators for boolean logic. Avoiding the short-circuiting is rarely important enough to use bitwise operators instead. And if you use it, remember it doesn't short-curcuit.

Dead code

Eliminate dead code to avoid confusing or irritating readers.
After break, return, and the like, additional code in the block without intervening jump-label (in general, goto is to be avoided, as you do) or case-label is unreachable.

Initialize on declaration

Unless you want to run under strict C89, (just about) all compilers support mixing declarations and statements, and if you ask for any optimization, the result is generally the same. Enhancing readability and just about eliminating the chance for a whole class of bugs for no cost is a bargain, right?

See "Is declaration of variables expensive?".

Help finding bugs

If base to itoa() is out-of-range, that is a programming-error which should be found and fixed in development, never to be seen in release-mode. assert() is designed for that.

If you really want, you might kill the program in release too, trading efficiency for defense in depth, but don't ever silently swallow the error. Doing so makes debugging a nightmare.

Be const-correct

Doing so enables the compiler to help you find bugs, and makes functions more self-documenting.

Use restrict where appropriate

Doing so allows the compiler to assume memory is not aliased, eliminating potential dependencies and increasing efficiency, in addition to making functions more self-documenting.

Line-length

Long lines are hard to read, especially if they lead to wrapping by the editor or horizontal scrolling.
172 is really a bit much, below 80 would be best.

String-literals are easily wrapped, as consecutive ones are merged by the compiler.

What is a string

A string is a sequence of non-nul elements including and terminated by the first nul element. str@@-functions are expected to handle strings, though there are infamous exceptions.

Single source of truth

The outer dimension of an array is sized according to the initializer if omitted. And you can get it back using (sizeof *array / sizeof array).

Unless there is a good reason to do otherwise, instead of an array of arrays of fixed length for storing string-literals, consider an array of pointers. It might be more efficient, and eliminates the forced fixed length.

switch and single label

An if-statements seems much more appropriate, as it is simpler.

Appropriate types

size_t is the designated type for object-sizes and array-indices. Unless you have a compelling reason, use it.

String length

Don't blindly assume strings are a minimum number of characters long. Empty strings exist, and out-of-bounds access is generally Undefined Behavior.

Additional comments for specific code

  1. Why should itoa() only supply the minus-sign if the base is 10?
    That doesn't seem to make any sense.

  2. strr() is just bad.

    • It looks more like a mem-function, as it doesn't treat the input as a string, nor outputs a string.
    • r for reverse is very cryptic. Just spell it out.
    • Copying elements x to y both exclusive is a very surprising contract. If you don't use a single length-parameter as expected (the caller can adjust the source-pointer appropriately), I would expect the first to be inclusive.
  3. equstr() is also an acquired taste.

    • It is not quite a string-function as it doesn't terminate the target.
    • It really threw me for a loop that it modifies one argument, instead of comparing them, with the semantics of !strcmp(a, b).
  4. clear_str()

    • nulling out all of a strings elements is rarely needed. Generally, just truncating it by setting the first element to zero is sufficient.
  5. is_string()

    Are you sure anything starting with and ending with " is a string according to your grammar?

    • What about embedded "?
    • What about the length one string "\""?
    • What about escape-sequences?
  6. is_sint()

    • Ramp up the warning level. Your compiler should warn about the path without return-statement.
    • Are you sure only the first character should be tested?
    • Are you sure a signed integer must not be negative?

    When fixing that, don't screw up the empty string.

  7. lang_arg_lens()

    • Where is the dynamic memory allocated freed again?
    • MAX_STRING_LENGTH seems wasteful.
\$\endgroup\$
2
\$\begingroup\$

is_string

int is_string(char *str)
{
    return str[0] == '"' && str[strlen(str) - 1] == '"';
}

Instead of returning TRUE or FALSE (as mentioned in another answer, use true and false), simply return the expression as it resolves to a boolean value anyway. It makes your code shorter and easier to scan through.

is_sint

int is_sint(char *str)
{
    return str[0] >= '0' && str[0] <= '9';
}

Since the while loop doesn't iterate a second time, and i is never incremented, so we only need to evaluate the expression once, and not in the loop. Uses the same logic as above.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.