5
\$\begingroup\$

Description

The code uses strtol() to convert text that represents a value in a given base into an integer. It takes three arguments from the command line, including the string and the base. The string may begin with an arbitrary amount of white space followed by a optional - or +. If the base is 0 or 16, the string may include 0x or 0X and it would be taken as hexadecimal, otherwise a zero base will be taken as decimal.

From strtol()'s manpage:

(In bases above 10, the letter 'A' in either uppercase or lowercase represents 10, 'B' represents 11, and so forth, with 'Z' representing 35.)

I compiled it under the flags -Wall -g -Winit-self -Wredundant-decls -Wfloat-equal -Winline -Wunreachable-code -Wmissing-declarations -Wswitch-default -Wmain -pedantic-errors -pedantic -Wfatal-errors -Wextra -Wall and it didn't issue any warnings.

Questions

  1. Style review (indentation, comments, readability, etc.)
  2. Is there room for more error-checking? Have I forgotten anything except for an invalid base (that I remembered now)?
  3. How could it be improved?

Sample input/output

Examples of successful conversions:

  1. ./a.out 102438 10
    Success [102438].
    
  2. ./a.out 0403 8
    Success [259].
    
  3. ./a.out 0x41 16
    Success [65].
    
  4. ./a.out 00101010 2.
    Success [42].
    

Examples of errors:

  1. ./a.out acbdavikf 2
    Error: No digits were found.
    
  2. ./a.out 123acbdavikf 0
    Error: Trailing junk after number.
    
  3. ./a.out 124235898259250270295 10
    strtol: Numerical result out of range
    
  4. ./a.out  -124235898259250270295 10
    strtol: Numerical result out of range
    

Code

/* The program converts any base number to its equivalent decimal number. */
 
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
 
static void convert_to_long(const char *nptr, const int base);
static void *error_checked_malloc(int size);
static void print_error_msg(const char *command);
 
int main(int argc, char *argv[])
{
    char *command = error_checked_malloc(64 * sizeof(char));
    command = argv[0];
 
    if (argc != 3)
        print_error_msg(command);
 
    char *str = argv[1];
    int base = atoi(argv[2]);

    convert_to_long(str, base);

    return EXIT_SUCCESS;
}
 
///===========================================================================================================================================================
static void convert_to_long(const char *nptr, const int base)
{
    char *endptr;
    errno = 0;     /* To distinguish success/failure after call. */
    long int val = strtol(nptr, &endptr, base);

    /* Check for various possible errors */
    if (nptr == endptr)
    {
        fprintf(stderr, "Error: No digits were found.\n");
        exit(EXIT_FAILURE);
    }

    if (*endptr != '\0')
    {
        fprintf(stderr, "Error: Trailing junk after number.\n");
        exit(EXIT_FAILURE);
    }

    if (errno == ERANGE)    /* Checks both over/under-flow */
    {
        perror("strtol");
        exit(EXIT_FAILURE);
    }

    else
    {
        printf("Success [%ld].\n", val);
        exit(EXIT_SUCCESS);
    }
}
 
///==================================================================================================================================================================
static void *error_checked_malloc(int size)
{
    char *ptr = malloc(size);
    if (!ptr)
    {
        perror("Malloc");
        exit(EXIT_FAILURE);
    }
    return ptr;
}
 
///=====================================================================================================================================================================
static void print_error_msg(const char *command)
{
    printf("Usage: <%s> <string> <base>\n", command);
    exit(EXIT_FAILURE);
}
\$\endgroup\$
5
  • 3
    \$\begingroup\$ Minor suggestion: the name of error_checked_malloc() sounds like it is meant to guard against things like use-after-frees and double-frees. In GNU code, it is conventional to call this function xmalloc, and I suggest you adopt this name \$\endgroup\$
    – JohnScott
    Nov 26, 2022 at 8:00
  • \$\begingroup\$ Apart from the zero case, the functions do match. Convention adopted. \$\endgroup\$
    – Harith
    Nov 26, 2022 at 11:11
  • \$\begingroup\$ Minor point: the code doesn't convert between bases; it converts text that represents a value in a given base into an integer. Neither the integer nor the value that it holds has a base. It's only when you display the integer (which the code in the question doesn't do) that a base is involved; you can display the value in base 2, base 8, base 10, base 16, or any other base you might like. \$\endgroup\$ Nov 27, 2022 at 17:15
  • \$\begingroup\$ @PeteBecker Yes, that's correct. I would change that claim in the question. \$\endgroup\$
    – Harith
    Nov 27, 2022 at 17:38
  • \$\begingroup\$ I don't have the time for a full answer, but the most grievous issue is that you do not separate the "core" logic from the I/O (driver). Try to write the conversion function that would be useful as a library => it'll allow you writing unit-tests, notably. \$\endgroup\$ Nov 28, 2022 at 18:11

3 Answers 3

7
\$\begingroup\$

Style review(indentation, comments, readability, etc.)

Noise

const serves no purpose with const int base in the static void convert_to_long(const char *nptr, const int base); declaration.

Simplify

// static void convert_to_long(const char *nptr, const int base);
static void convert_to_long(const char *nptr, int base);

const with an object has limited value in function definitions.

Allocate to the referenced object, not the type

Allocating to the refenced object is easier to code right, review and maintain.

// command = error_checked_malloc(64 * sizeof(char));
command = error_checked_malloc(64 * sizeof command[0]);

else not needed

Simplify

//else
//{
//    printf("Success [%ld].\n", val);
//    exit(EXIT_SUCCESS);
//}

printf("Success [%ld].\n", val);
exit(EXIT_SUCCESS);

exit(EXIT_SUCCESS); is troublesome in a helper function.

Better to return success from main().

Excessive vertical white-space

Inconsistent indent

if (argc != 3) print_error_msg(command); deserves {}.


Is there room for more error-checking? Have I forgotten anything except for an invalid base(that I remembered now)?

Error messages deserve to print on stderr

// printf("Usage: <%s> <string> <base>\n", command);
fprintf(stderr, "Usage: <%s> <string> <base>\n", command);

atoi() lacks checking

Non-numeric input, numeric text input with trailing text and out of int range values expose weakness with the simplistic aoti(). Consider strtol().

base not in expected range

With strtol(nptr, &endptr, base);, a base value of negative, 1, more than 36 are all problematic and undetected here.

Maintain case

// perror("Malloc");
perror("malloc");

How could it be improved?

Allow trailing whitespace

Instead of if (*endptr != '\0'), perhaps first march down the string if it has optional trailing white-space. This simplifies testing a line of input from fgets().

Allocation deserves to use type size_t for size, not int

Zero

error_checked_malloc(0) should not result in an error.

Negative

error_checked_malloc(some negative value) should certainly result in an error. As is, code may successfully allocate a huge block.

convert_to_long() surprisingly prints

Aside from errors, I'd expect convert_to_long() to convert and let the calling code do any printing.

Allow input to omit base

When base argument missing, simply assume 0 (or 10).

\$\endgroup\$
4
  • \$\begingroup\$ You raised some good points. I wasn't aware of the other complications pertaining to malloc, but would it be applicable to test for a negative value in this context? Could the filename/command be rendered negative somehow? \$\endgroup\$
    – Harith
    Nov 26, 2022 at 10:30
  • \$\begingroup\$ @HarisSalam As size_t is some unsigned type, perhaps static void *error_checked_malloc(size_t size) { if (size == 0) { return NULL; } char *ptr = malloc(size); if (!ptr) { perror("malloc"); exit(EXIT_FAILURE); } return ptr; } \$\endgroup\$ Nov 26, 2022 at 10:41
  • \$\begingroup\$ "Could the filename/command be rendered negative somehow?" is unclear. Please restate. \$\endgroup\$ Nov 26, 2022 at 10:42
  • \$\begingroup\$ I was referring to the NEGATIVE section as size_t couldn't be unsigned and malloc could allocate a huge block of memory successfully. I got it now. Thank you. \$\endgroup\$
    – Harith
    Nov 26, 2022 at 11:10
7
\$\begingroup\$

chux and Toby already made many excellent points. I want to add one:

Make the output useful by being less verbose

Being verbose when printing an error message is helpful, but don't be verbose when your program is running successfully. Consider that someone might want to use your program in a script, or perhaps that you as a user want to copy&paste the result to somewhere else. If you print:

Success [42].

Then it's harder to copy&paste (since it's now easier to accidentily select unwanted characters on this line), but it's especially harder to use in a script: if you are just interested in the resulting value 42, then you'd have to run the output of your program through some other commands just to strip away the Success [ and ].. Just output the value without anything else:

42
\$\endgroup\$
0
6
\$\begingroup\$

Everything that chux said, and:

The memory allocation is unused and leaked. Look at this:

    char *command = error_checked_malloc(64 * sizeof(char));

    command = argv[0];

Now that command points to the zeroth argument, there's no way to access or free the memory we allocated. Replace this with

    char *command = argv[0];

BTW, sizeof (char) is necessarily 1 (since sizeof measures in units of char), so that multiplication is just noise.

\$\endgroup\$
1
  • \$\begingroup\$ Now I understand why valgrind was complaining about freeing illegal memory. Thanks, Toby. \$\endgroup\$
    – Harith
    Nov 26, 2022 at 10:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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