12
\$\begingroup\$

I'm practicing C and wrote a function to trim leading/trailing spaces in a string:

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

void string_trim(char **input) {
  char *input_copy = strdup(*input);
  int input_length = strlen(input_copy);

  int start_index = 0;
  while (*(input_copy++) == ' ') {
    start_index++;
  }
  input_copy -= start_index + 1;

  int end_index = input_length;
  input_copy += input_length - 1;
  while (*(input_copy--) == ' ') {
    end_index--;
  }
  input_copy -= input_length - end_index + 1;

  int new_length = end_index - start_index;
  *input = realloc(*input, sizeof(char) * new_length + 1);

  int index = 0;
  for (int i = start_index; i < end_index; i++) {
    (*input)[index] = input_copy[i];
    index++;
  }
  (*input)[index] = '\0';

  free(input_copy);
}

int main() {
  char *str = malloc(sizeof(char) * 256);
  strcpy(str, "  hello    ");

  string_trim(&str);

  printf("Result: '%s'\n", str);

  free(str);
}

This works, but feels convoluted, is there a cleaner of doing it? Also, reallocating pointer that was passed to a function and allocated elsewhere first - I'm suspecting this would not be considered good practice or am I wrong?

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Why ignore the return value of library functions? \$\endgroup\$
    – Harith
    Feb 13, 2023 at 18:49
  • 1
    \$\begingroup\$ I feel that the strdup and/or the realloc is unnecessary. \$\endgroup\$
    – CPlus
    Feb 14, 2023 at 15:25
  • \$\begingroup\$ @Haris do you mean why string_trim does not return anything? \$\endgroup\$ Feb 16, 2023 at 13:29
  • \$\begingroup\$ No, I am talking about the return values of realloc() and malloc(). \$\endgroup\$
    – Harith
    Feb 16, 2023 at 15:32

10 Answers 10

5
\$\begingroup\$

You Have Several Buffer Overrun Bugs

If we pass in a pointer that isn’t properly terminated, this function will gleefully copy past the end of the input buffer, copying whatever is after the string into the new buffer. This could both cause a security bug (similar to Heartbleed) and potentially waste gigabytes of memory.

Always, always, always check for buffer overruns in C! It is never too soon to learn good habits!

Sizes are size_t, not int

On most 64-bit systems, a size_t is unsigned and 64-bits wide, and an int is signed and 32-bits wide. There are a huge number of bugs you can cause by converting a size_t to an int or vice versa. If your compiler does not warn you about this line:

int input_length = strlen(input_copy);

you need to enable more warnings. On GCC, Clang or ICX, I typically use -Wall -Wextra -Wpedantic -Wconversion -Wdeprecated and a -std= option. (It’s -Wconversion that should flag this.) On other compilers, check the documentation. Ideally, turn off warnings for headers from other projects, and compile with -Werror as well.

Check the Buffer Size

It’s good practice in C to pass in the maximum size of the input buffer, so that, even if the input array is unterminated, the function cannot overrun its buffer.

Return a String Slice

I agree with the other answers that you should be using a different API. I disagree that you should be making a deep copy of the string at all. In this case, you can represent the trimmed string as a slice of the original. This is both safer and more optimized.

You can still easily make a deep copy of the string slice if you need to—but you often won’t need to. And your program will run much faster and use less memory if you can avoid it.

So let’s say we define this data structure:

// A const string slice:
typedef struct str_cslice_t {
  const char* s;
  size_t n;
} str_cslice_t;

Here’s some code demonstrating two ways we could output a slice without making a copy:

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

// A const string slice:
typedef struct str_cslice_t {
  const char* s;
  size_t n;
} str_cslice_t;


#define BUF_SIZE 1024U

// Not re-entrant:
int main(void)
{
  static char buffer[BUF_SIZE] = "   foo bar ";
  const str_cslice_t slice = { &buffer[3], 7 };

  assert( slice.n <= INT_MAX);
  printf( "\"%.*s\"\n", (int)slice.n, slice.s );

  memmove( buffer, slice.s, slice.n ); // Invalidates slice.s!
  buffer[slice.n] = '\0';
  printf( "\"%s\"\n", buffer );

  return EXIT_SUCCESS;
}

Most good C APIs (and also the ANSI C standard library) let you pass in either the length or the maximum length of your string. This is primarily to make it at least theoretically possible to write C code without memory bugs, but it’s often faster too. Here, I used the %.*s format specifier to pass in the maximum length of the string, which lets us print a string slice. (A dangerous piece of technical debt in this interface is that the maximum length is passed as the “precision,” which has type int. Just on principle, I add an assertion that this cast will not overflow. The compiler can optimize it out at runtime. An alternative would be to use fwrite.)

After that, I re-used the original buffer by moving the slice to the beginning and then truncating it. You can re-use your input buffers this same way any time you no longer need the untrimmed string. That’s a lot of the time.

You can do most other things you could do with a deep copy of the string, using a string slice, as well. It’s normally faster and uses less memory, too! For example, you can concatenate slices with memcpy rather than strings with strncat.

If you really, truly need a deep copy of the slice, you can call strndup.

Is Checking for that One Byte Enough?

The program is certainly simpler if you can just search for that one byte. Unfortunately, in the twenty-first century, supporting ASCII doesn’t cut it, but the standard library’s support for internationalization is stuck in the ’90s (and Microsoft’s does not even support that). To work with UTF-8, you would in practice need to use some third-party library, such as libunicode.

Fortunately, if searching for the space character (or other ASCII whitespace) is enough, the same code will also run correctly on UTF-8 input. I’ll assume that’s the case for now.

Putting it All Together

First, something simpler and closer to your code than my original solutions.

With my suggested API, the signature of trim_string becomes:

str_cslice_t string_trim( const char* const s, const size_t n )

If we add a function to convert a null-terminated string to_str_cslice, we decompose the problem into converting, and then calling a function to trim both ends of a string slice. The first is an important utility function to have anyway, and the second is simpler to write, since it doesn’t need to allocate any memory, copy anything or check the length.

#include <assert.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>

/* A const string slice:  Note that n is the actual length of the skice.
 * Slices ARE NOT null-terminated.
 */
typedef struct str_cslice_t {
  const char* s;
  size_t n;
} str_cslice_t;

const str_cslice_t EMPTY_STR_CSLICE = { NULL, 0 };


/* Trims leading and trailing whitespace from a str_cslice_t.
 */
str_cslice_t str_cslice_trim( const str_cslice_t input )
{
  if (input.s && input.n) {
    const char* const end = input.s + input.n;
  
    const char* first = input.s;
    while ( first < end && *first == ' ' ) {
      ++first;
    }

    // We previously checked that n > 0.
    const char *last = end - 1;
    while ( last > first && *last == ' ' ) {
      --last;
    }

    if (last > first || first > input.s) {
      const str_cslice_t result = { first,
                                    (size_t)(last - first + 1) };
      return result;
    }
  }

  return EMPTY_STR_CSLICE;
}

/* Converts a null-terminated string with a maximum length of n to a
 * str_cslice_t.
 */
str_cslice_t to_str_cslice( const char* const s, const size_t n )
{
  const size_t actual = strlen(s);
  const str_cslice_t result = { s, actual < n ? actual : n };
  return result;
}


/* Returns either a slice of the input string that does not include any
 * leading and trailing spaces, or an empty slice if that substring is empty.
 * The string is null-terminated, but its maximum length is n.
 */
str_cslice_t string_trim( const char* const s, const size_t n )
{
  return str_cslice_trim(to_str_cslice(s, n));
}


#include <limits.h>
#include <stdio.h>

// Not re-entrant:
int main(void)
{
  {
    static const char input[] = "   foo bar ";
    const str_cslice_t trim_both = string_trim( input, sizeof(input) );
    assert( trim_both.n <= INT_MAX );
    printf( "\"%.*s\"\n", (int)trim_both.n, trim_both.s );
  }

  {
    static const char input[] = "foo bar   ";
    const str_cslice_t trim_right = string_trim( input, sizeof(input) );
    assert( trim_right.n <= INT_MAX );
    printf( "\"%.*s\"\n", (int)trim_right.n, trim_right.s );
  }

  {
    static const char input[] = "   foo bar";
    const str_cslice_t trim_left = string_trim( input, UINT_MAX );
    assert( trim_left.n <= INT_MAX );
    printf( "\"%.*s\"\n", (int)trim_left.n, trim_left.s );
  }

  {
    static const char input[] = "      ";
    const str_cslice_t empty = string_trim( input, sizeof(input) );
    assert(empty.n == 0);
    printf( "\"%.*s\"\n", (int)empty.n, empty.s );
  }

  {
    static const char input[] = " ! ";
    const str_cslice_t singleton = string_trim( input, sizeof(input) );
    assert(singleton.n == 1);
    printf( "\"%.*s\"\n", (int)singleton.n, singleton.s );
  }

  {
    const str_cslice_t should_fail = string_trim( "", 0 );
    assert( !should_fail.s && !should_fail.n );
  }

  {
    const str_cslice_t should_fail = string_trim( NULL, UINT_MAX );
    assert( !should_fail.s && !should_fail.n );
  }

  return EXIT_SUCCESS;
}

I include a simple test harness at the bottom, but it does not have complete code coverage. Caveat emptor.

A Refactored Version

I belatedly realized that I could clean up the code immensely by factoring out the trim-left and trim-right functions. We can also make both of these library functions take and return our str_cslice_t structures, same as before. The string_trim function is now the composition of three smaller functions, all of which are somewhat useful on their own: converting a null-terminated string into a string slice, trimming it on the left, and trimming it on the right.

Because I’m weird, I like to code in a functional style even in C. This can get complicated (as in my previous edits), but here, with the functions small enough, it becomes very straightforward. One advantage of this approach is that the entire program can be written with static single assignments. The local state is always updated together, at the same time. This eliminates several large categories of bugs.

Since this approach makes heavy use of tail recursion, and C was not designed for that, you need to make sure that the compiler optimizes tail calls. Clang and ICX have an extension for this, so I #define MUSTTAIL to expand to that attribute on those compilers, or to a no-op on other compilers. On GCC, you will need to compile this code with -O2, -O3, -Os, or at least -foptimize-sibling-calls.

#include <assert.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>

#if __clang__ || __INTEL_LLVM_COMPILER 
#  define MUSTTAIL __attribute((musttail))
#else
#  define MUSTTAIL /**/
#endif

/* A const string slice:  Note that n is the actual length of the skice.
 * Slices ARE NOT null-terminated.
 */
typedef struct str_cslice_t {
  const char* s;
  size_t n;
} str_cslice_t;

const str_cslice_t EMPTY_STR_CSLICE = { NULL, 0 };


/* Trims leading whitespace from a slice.
 */
str_cslice_t str_cslice_trim_left( const str_cslice_t input )
{
  if (!input.s || !input.n)
    return EMPTY_STR_CSLICE;

  if (*input.s != ' ')
    return input;
    
  MUSTTAIL return str_cslice_trim_left((struct str_cslice_t){ input.s+1, input.n-1 });
}


/* Trims trailing whitespace from a slice.
 */
str_cslice_t str_cslice_trim_right( const str_cslice_t input )
{
  if (!input.s || !input.n)
    return EMPTY_STR_CSLICE;

  if (input.s[input.n-1] != ' ')
    return input;

  MUSTTAIL return str_cslice_trim_right((struct str_cslice_t){ input.s, input.n-1 });
}

/* Trims leading and trailing whitespace from a str_cslice_t.
 */
str_cslice_t str_cslice_trim( const str_cslice_t input )
{
  MUSTTAIL return str_cslice_trim_right(str_cslice_trim_left(input));
}


/* Converts a null-terminated string with a maximum length of n to a
 * str_cslice_t.
 */
str_cslice_t to_str_cslice( const char* const s, const size_t n )
{
  const size_t actual = strlen(s);
  const str_cslice_t result = { s, actual < n ? actual : n };
  return result;
}


/* Returns either a slice of the input string that does not include any
 * leading and trailing spaces, or an empty slice if that substring is empty.
 * The string is null-terminated, but its maximum length is n.
 */
str_cslice_t string_trim( const char* const s, const size_t n )
{
  return str_cslice_trim(to_str_cslice(s, n));
}


#include <limits.h>
#include <stdio.h>

// Not re-entrant:
int main(void)
{
  {
    static const char input[] = "   foo bar ";
    const str_cslice_t trim_both = string_trim( input, sizeof(input) );
    assert( trim_both.n <= INT_MAX );
    printf( "\"%.*s\"\n", (int)trim_both.n, trim_both.s );
  }

  {
    static const char input[] = "foo bar   ";
    const str_cslice_t trim_right = string_trim( input, sizeof(input) );
    assert( trim_right.n <= INT_MAX );
    printf( "\"%.*s\"\n", (int)trim_right.n, trim_right.s );
  }

  {
    static const char input[] = "   foo bar";
    const str_cslice_t trim_left = string_trim( input, UINT_MAX );
    assert( trim_left.n <= INT_MAX );
    printf( "\"%.*s\"\n", (int)trim_left.n, trim_left.s );
  }

  {
    static const char input[] = "      ";
    const str_cslice_t empty = string_trim( input, sizeof(input) );
    assert(empty.n == 0);
    printf( "\"%.*s\"\n", (int)empty.n, empty.s );
  }

  {
    static const char input[] = " ! ";
    const str_cslice_t singleton = string_trim( input, sizeof(input) );
    assert(singleton.n == 1);
    printf( "\"%.*s\"\n", (int)singleton.n, singleton.s );
  }

  {
    const str_cslice_t should_fail = string_trim( "", 0 );
    assert( !should_fail.s && !should_fail.n );
  }

  {
    const str_cslice_t should_fail = string_trim( NULL, UINT_MAX );
    assert( !should_fail.s && !should_fail.n );
  }

  return EXIT_SUCCESS;
}

ICX 2022 with -std=c17 -O3 -march=x86-64-v3 is able to inline these calls, resulting in extremely efficient code. Each of the test cases compiles to a block like the following:

        mov     edi, offset .L.str.2
        mov     edx, offset main.input+3
        mov     esi, 7
        xor     eax, eax
        call    printf

The non-inlined version optimizes into a tight loop as well, and this version does not use the heap at all, nor make a deep copy of any string.

One last footnote: if you are extremely security-conscious, you might want to use strnlen_s rather than strlen in the to_str_cslice function. This version is very fast and portable, but it’s theoretically possible that an unterminated string could make strlen run off the end of the buffer onto some unreadable page of memory and crash the program.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Good review, but I disagree on a specific aspect. If we pass in a pointer that isn’t properly terminated (or a null pointer), then we have violated the contract of the function - it's documented to accept a string, so it's reasonable to declare that it has undefined behaviour if passed anything else. It's not normal to pass a size limit for string input (output is a different matter) and it's a good idea to use familiar interfaces where reasonable. \$\endgroup\$ Feb 15, 2023 at 8:14
  • \$\begingroup\$ The "string slice" object is certainly a valuable concept, and well explained here. In practice, we often need two versions, for views of both mutable and const characters. \$\endgroup\$ Feb 15, 2023 at 8:21
  • 1
    \$\begingroup\$ @TobySpeight "so it's reasonable to declare that it has undefined behaviour if passed anything else." --> well some folks like to do some tests like if the string pointer is NULL, even though NULL does not point to a string. ;-) \$\endgroup\$ Feb 15, 2023 at 15:50
  • \$\begingroup\$ Yes @chux - and irony noted! I don't usually claim that's supported or even documented as allowed input, just reasonable defensiveness. (You'll notice in the linked answer no attempt to write through the output parameter in that case, unlike with valid input) \$\endgroup\$ Feb 15, 2023 at 15:58
  • 1
    \$\begingroup\$ @TobySpeight I think memory management in C requires more defensive coding than we usually see. Shrugging and saying, “It’s their responsibility to ensure that they only use this function the right way,” is how we got where we are today. Reducing the attack surface matters. If we’re passing around strings in C without keeping track of what size the buffers are supposed to be, we are already in a state of mortal sin. \$\endgroup\$
    – Davislor
    Feb 15, 2023 at 16:34
14
\$\begingroup\$

There are indeed a few things that can be improved.

Simplify your argument

There is no need to take a char**, a simple char* is enough.

Leave allocations to the caller whenever possible

It is easy for a caller to provide an output buffer, a scratch buffer, etc... so it's best to leave it to the caller to do so in general.

This allows the caller to pass pointers to stack-allocated (or static) buffers, to use different (specialized) allocators, to reuse the same buffer across calls, etc... if they so wish.

In your case, you can even just modify the input in-place, so you may as well.

Ask the caller for the length of the string

Similarly, it is easy for the caller to provide the length of the string. This has multiple advantages:

  1. Efficiency: if the caller has calculated the length already, or will need to calculate it afterwards, this saves one calculation.
  2. Flexibility: if the caller has a non-NUL terminated buffer, they don't need to terminate it. Or if the caller has a string with embedded NUL characters, they can still use your routine.

Use standard copying routines

There are a multitude of existing standard library functions to copy memory: memcpy and memmove, strcpy, ...

Using them will simplify your code, better indicating your intent, and will result in faster code as they are heavily optimized.

Document the pre-conditions

C does not specify the encoding of its strings (Latin-1? JS-SHIFT? UTF-8?), so it's up to you to explain which encodings your function support.

Document the algorithmic complexity of your code

It's good practice to explain the algorithmic complexity of your code.

It's useful for callers to get an idea of its performance, and it's a good sanity check for you, the writer, that you didn't accidentally blow-up.

Putting it altogether

Here is an alternative implementation taking all the above into account.

#include <string.h>

/// Removes leading and trailing whitespaces (0x20), in place.
///
/// Returns the length of the newly trimmed string.
///
/// # Pre-Conditions
///
/// The string is assumed to be Latin-1 or UTF-8 encoded, other encodings may not work.
///
/// # Complexity
///
/// O(length) in time and O(1) in extra space.
size_t string_trim(char* input, size_t length) {
    char* start = input;
    char* end = input + length;

    for (; start < end && *start == ' '; ++start) {}
    for (; end > input && end[-1] == ' '; --end) {}

    size_t new_length = end - start;

    memmove(input, start, new_length);

    return new_length;
}

And write tests

I don't see any test for your function, and I really advise you to write some, considering edge-cases:

  • Empty string.
  • All whitespace string.
  • All combinations of with many/with a single/without leading whitespace, with many/with a single/without trailing whitespace, and with/without inner whitespace.
  • Other whitespacey leading/trailing characters (\r\n\t...).
\$\endgroup\$
9
  • 1
    \$\begingroup\$ @chux-ReinstateMonica: memmove is intentional, the input need not be NUL-terminated, and the output will not be NUL-terminated. In case there's nothing to trim, there is not necessarily any space to put a NUL character at the end. \$\endgroup\$ Feb 14, 2023 at 13:32
  • 1
    \$\begingroup\$ @chux-ReinstateMonica: I added examples of encodings, making it clearer. \$\endgroup\$ Feb 14, 2023 at 13:33
  • 3
    \$\begingroup\$ "the input need not be NUL-terminated, and the output will not be NUL-terminated" OK, but then do not comment "Returns the length of the newly trimmed string." as a string in C, given "A string is a contiguous sequence of characters terminated by and including the first null character." always has a null character. Note that OP's goal is "Trim ... a string" and uses code that relies on a null character. \$\endgroup\$ Feb 14, 2023 at 13:44
  • 1
    \$\begingroup\$ @chux-ReinstateMonica: Good point on missing the size check, will fix. \$\endgroup\$ Feb 15, 2023 at 7:42
  • 1
    \$\begingroup\$ Leave allocations to the caller whenever possible +1 for that one :) \$\endgroup\$
    – Marco
    Feb 16, 2023 at 19:20
8
\$\begingroup\$

General Observations

You're correct it is too complex. As J_H indicates the the API is problematic, although I disagree with how J_H suggested the API should be implemented. My suggestion is that the function should return a new string with the modifications necessary.

I feel that you are calling strdup() too early.

You could break the function up into sub functions, find the first nonspace character, find the last nonspace character, temporarily terminate the string after the last nonspace character and then do the strdup().

FYI, it would be better if input_length was declared as a size_t since that is what strlen() returns and I get a warning message on this line:

    int input_length = strlen(input_copy);

warning: 'initializing': conversion from 'size_t' to 'int', possible loss of data

Test for Possible Memory Allocation Errors

Since strdup() calls malloc() it is possible the memory allocation can fail, and then strdup() will return NULL.

In modern high-level languages such as C++, memory allocation errors throw an exception that the programmer can catch. This is not the case in the C programming language. While it is rare in modern computers because there is so much memory, memory allocation can fail, especially if the code is working in a limited memory application such as embedded control systems. In the C programming language when memory allocation fails, the functions malloc(), calloc() and realloc() return NULL. Referencing any memory address through a NULL pointer results in undefined behavior (UB).

Possible unknown behavior in this case can be a memory page error (in Unix this would be call Segmentation Violation), corrupted data in the program and in very old computers it could even cause the computer to reboot (corruption of the stack pointer).

To prevent this undefined behavior a best practice is to always follow the memory allocation statement with a test that the pointer that was returned is not NULL.

Alternate Algo

Some what simpler.

char* string_trim(char* input) {
    if (!input)
    {
        fprintf(stderr, "In string_trim() programmer error input is NULL");
        return NULL;
    }
    char* first_non_space = input;
    char* last_non_space = &input[strlen(input) - 1];

    while (*first_non_space == ' ')
    {
        first_non_space++;
    }

    while (*last_non_space == ' ')
    {
        --last_non_space;
    }
    last_non_space++;
    char temp = *last_non_space;
    *last_non_space = '\0';

    char* trimmed_string = strdup(first_non_space);
    if (!trimmed_string)
    {
        fprintf(stderr, "strdup failed in string_trim");
        return NULL;
    }
    *last_non_space = temp;

    return trimmed_string;
}
\$\endgroup\$
7
  • 3
    \$\begingroup\$ input[strlen(input) - 1] is UB when input[0] == 0. \$\endgroup\$ Feb 14, 2023 at 14:01
  • \$\begingroup\$ Seems strange to modify the original string and then return a duplicate. I'd expect either modify in place or if duplicating, leave the original string as is and consider strndup() to form a shorten copy. \$\endgroup\$ Feb 14, 2023 at 14:07
  • \$\begingroup\$ @chux-ReinstateMonica Added some missing error checking to prevent UB. \$\endgroup\$
    – pacmaninbw
    Feb 14, 2023 at 15:07
  • \$\begingroup\$ Even with updates, UB potential exists. if (!input) does not prevent input[0] == 0 (pointer compare vs. character compare). \$\endgroup\$ Feb 14, 2023 at 17:26
  • \$\begingroup\$ @chux-ReinstateMonica would if (!input || !*input) return NULL do the trick? \$\endgroup\$
    – pacmaninbw
    Feb 14, 2023 at 17:44
7
\$\begingroup\$

void string_trim(char **input) is an in place trimming.

Can't alter *input

Calling code need str to point to the same location before and after calling string_trim() for free(str) to work.

  char *str = malloc(sizeof(char) * 256);
  ...
  string_trim(&str); // Pointer str should not change.
  ...
  free(str);

No need for malloc() in string_trim()

Simply shift and truncate the string in one pass.

Wrong indexing type

String lengths can exceed INT_MAX. Use size_t.

Only a single pass down the string is needed

OP's code looks like 5 passes through the string: strdup:2, strlen, while()s, for().

Avoid future name collisions

Function names that begin with str, mem, or wcs and a lowercase letter may be added to the declarations in the <string.h> header. C17dr § 7.31.13 1

Perhaps str_trim()?

Space vs. white-space

Usually isspace() is used instead of ch == ' ' to also match tab, new-line, form_feed, etc.

Useful to consider the empty string

OP's code then fails with input_copy -= start_index + 1; as input_copy -= 0 + 1; is undefined behavior (UB) as address computations are limited to the (allocated) object's memory or "one-pass".


Sample (untested) alternative:

char* str_trim(char *input) {  // Note: char *, not char **
  // Skip leading spaces
  const char *src = input;
  char *dest = input;
  while (*src == ' ') {
    src++;
  }

  // Walk rest of string
  char *space_pointer = NULL;
  while (*src) {
    // If a space ...
    if (*src == ' ') {
      // Note its location in the shifted string if it begin a new space sequence.
      if (space_pointer == NULL) {
        space_pointer = dest;
      }
    } else {
      space_pointer = NULL;
    }
    // Copy the current character to its potential new location.
    *dest++ = *src++;
  }
  // Determine where to end the string.
  if (space_pointer) {
    *space_pointer = '\0';
  } else {
    *dest = '\0';
  }
  return input;
}
\$\endgroup\$
6
\$\begingroup\$
void string_trim(char **input) {

This works, but it's an odd API design. A more natural signature would use a struct instead of char **, or would return a start index:

// Writes a NUL so input has no trailing blanks,
// and fills in start_index to point at first non-blank character.
void string_trim(char *input, size_t *start_index) {

Your signature was missing accompanying documentation about side effects on pointer and on string contents.


  while (*(input_copy++) == ' ') {
    start_index++;
  }
  input_copy -= start_index + 1;

That fixup at the end is just weird. Better to allocate a new temp variable for the loop. Give it a concise name like s or p.


  while (*(input_copy--) == ' ') {

Here is a subtle point. We should document that we only handle ASCII, or C locale, strings. I doubt that this loop is correct when the string ends with UTF8 Chinese codepoints. Some of them will have a multibyte encoding that ends with 0x20, and NULing that out would be trouble, it would corrupt a properly encoded sequence. Don't change the code. Just document the assumptions.

To avoid such pitfalls, it would be necessary to decode some glyphs at the end of the string, to synchronize with the UTF8 encoding.


  *input = realloc(*input, sizeof(char) * new_length + 1);

Wow, this is really weird, don't do that. Caller allocated an object, and you just potentially moved it to another location?!? Yikes.

If you want to allocate a new object, return it, and make caller responsible for its lifecycle, fine. You can even realloc() it prior to returning, if you like. But this business of messing with caller's allocation seems like trouble. And hard to test, given the latitude realloc() has for returning same or different memory address.

I advise to not call realloc(); NUL out the new end of string and wait for caller to eventually free the whole thing.

The contents will be unchanged in the range from the start of the region up to the minimum of the old and new sizes.

Realloc() always has the freedom to change the memory location if it wants to. (For example, a debug version might choose to do that.) The contract is about the contents pointed at, rather than about the pointer.

Consider this. Caller might have allocated p = malloc(20) and assigned q = p + 10. Then caller trims blanks and you wind up reallocating to a brand new memory location. So q is pointing at free'd garbage. You (A.) need to advise caller of this in the docs, and (B.) shouldn't be messing with caller's allocation in the first place, let him worry about the lifecycle, just return a start_index into the existing object.

Also, if your API doesn't return a new start_index then you're stuck with copying, and there's no way you're going to copy a long string faster than memcpy(). So use that. It will vectorize nicely according to what the target CPU supports.


This code base began with questionable API design choices.

The implemented code does not appear to behave correctly.

Automated unit tests do not yet examine the various corner cases.

There is a paucity of documentation for the observable side effects.

This code is not yet ready to go into production.

\$\endgroup\$
4
  • 3
    \$\begingroup\$ "Some of them will have a multibyte encoding that ends with 0x20": no, all multibyte sequences in UTF-8 have the high bit set \$\endgroup\$
    – lights0123
    Feb 14, 2023 at 4:45
  • \$\begingroup\$ Your platform's man page likely tells you about the implementation of functions on that platform. To learn about behaviour you can portably depend upon, you should read the language specification (or documentation specifically derived from that - e.g. cppreference). In this case, the specification of realloc() makes no guarantees that shrinking returns the input pointer. \$\endgroup\$ Feb 14, 2023 at 15:43
  • \$\begingroup\$ Specific implementations are free to make such guarantees, but code that depends on them is no longer portable (which may or may not be acceptable). \$\endgroup\$ Feb 14, 2023 at 15:44
  • 1
    \$\begingroup\$ Using realloc(input) is really bad - it means the caller can pass only strings that are allocated in dynamic memory, and not with (e.g.) a local char[] buffer. \$\endgroup\$ Feb 15, 2023 at 9:51
4
\$\begingroup\$

Start with good tests

It's good that we have a usage example in main(). We can do a lot better:

  • Test more values: make sure it works with or without spaces at each end; also test empty and all-space inputs.
  • Use the exit status to indicate whether the tests produce expected results.

This is how I'd start:

/* return 0 for success, 1 for failure */
static int test_trim(const char *input, const char *expected)
{
    size_t len = strlen(input);
    char s[len+1];          /* Variable-length array - requires C99 */
    memcpy(s, input, len+1);
    char *p = s;
    string_trim(&p);
    return strcmp(s, expected) != 0;
}

int main(void)
{
    return test_trim("", "")
        |  test_trim(" ", "");
}

We can add more tests as we go, but before we do, let's look at the function interface.

Improve the interface

Firstly, the name string_trim is reserved by the C standard which says that in future versions of C:

Function names that begin with str, mem, or wcs and a lowercase letter may be added to the declarations in the <string.h> header.

Secondly, it's probably useful to accept char* and return a pointer to the start of the trimmed string:

char *trimmed(char *);

One advantage to this interface is that we can return a pointer to the first non-whitespace character without moving the data to the start - that removes a significant portion of the code and the running time.

If the caller wants to copy or move the trimmed string, then it will want to know the length of the string. This is something we find as a side-effect, so we could provide that via an "out" parameter if desired:

char *trimmed(char *input, size_t *trimmed_size)
{
    ⋮
    if (trimmed_size) {
        *trimmed_size = end_index - start_index;
    }
}

In fact, it's probably a good idea to split into two functions, so we can find the bounds of a read-only string with the intention of copying it or using with length-limited functions.

At this stage, we have the following code (with a dummy implementation that just returns the string unchanged, and therefore fails the second test):

const char *trimmed_bound(const char *input, size_t *trimmed_size)
{
    if (trimmed_size) {
        *trimmed_size = strlen(input);
    }
    return input;
}

char *trimmed_string(char *input, size_t *trimmed_size)
{
    size_t len;
    if (!trimmed_size) {
        trimmed_size = &len;
    }
    char *s = (char*)trimmed_bound(input, trimmed_size);
    s[*trimmed_size] = '\0';
    return s;
}
/* return 0 for success, 1 for failure */
static int test_trim(const char *file, int line,
                     const char *input, const char *expected)
{
    size_t len;
    const char *s = trimmed_bound(input, &len);
    if (len == strlen(expected) && memcmp(s, expected, len) == 0) {
        /* test passed */
        return 0;
    }
    fprintf(stderr, "%s:%d: trim('%s') should yield '%s' but got '%.*s'\n",
            file, line, input, expected, (int)len, s);
    return 1;
}

#define TEST_TRIM(input, expected) test_trim(__FILE__, __LINE__, input, expected)

int main(void)
{
    return TEST_TRIM("", "")
        |  TEST_TRIM(" ", "");
}

I've wrapped the test_trim helper with a macro to print the test case location - this is useful in editors such as Emacs which can parse error messages and locate the code in question.


Think with pointers

Now we need to make the tests pass. I'll start from scratch here.

The core of the function is to find two positions in the string:

  1. The first non-space character, if there is one;
  2. The last non-space character, again if there is one.

We can do this with a single pass over the string. For each character we visit, ignore it if it's a space. If it's non-space, then set pointer to first if not already set, and set pointer to last (always). When we reach the end of the string, the pointers are either unset (empty result) or point to the first and last non-space character respectively.

Let's add a full set of all our test cases:

int main(void)
{
    return TEST_TRIM("", "")
        |  TEST_TRIM(" ", "")
        |  TEST_TRIM("  ", "")
        |  TEST_TRIM("foo bar", "foo bar")
        |  TEST_TRIM(" foo bar", "foo bar")
        |  TEST_TRIM("  foo bar", "foo bar")
        |  TEST_TRIM("  bar foo  ", "bar foo")
        |  TEST_TRIM("foo bar  ", "foo bar");
}

The implementation using pointers starts like this:

    const char *first = NULL;
    const char *last = NULL;

    for (;  *p;  ++p) {
        if (*p != ' ') {
            if (!first) { first = p; }
            last = p + 1;
        }
    }

When we reach the end of the string, we have either set both pointers or none. If we have set both pointers, the result length is just the difference between them: (size_t)(last - first).

If first is unset, we haven't seen any non-space characters. So we want the length to be zero. A simple way to achieve this is to point both first and last to the the terminating null character, which is conveniently where we left p pointing:

    if (!first) {
        /* empty result */
        first = last = p;
    }

Consider removing all whitespace

The <ctype.h> header provides a useful function to test whether a character is in the set of "space" characters (space, tab, newline, carriage return, etc). If we want a function that strips all these, we can use isspace() - but we can't pass a a plain (possibly signed) char. Its argument is a positive integer (or EOF, but that's not applicable here), so we need to convert to unsigned char before it's widened to int:

    const unsigned char *first = NULL;
    const unsigned char *last = NULL;
    const unsigned char *p = (const unsigned char *)input;
    for (;  *p;  ++p) {
        if (!isspace(*p)) {
            if (!first) { first = p; }
            last = p + 1;
        }
    }

Refactor

With a good set of tests in place, we can experiment with confidence.
One thing to try is to to separate the search for first and last characters:

    const unsigned char *p = (const unsigned char *)input;
    while (isspace(*p)) {
        ++p;
    }
    const unsigned char *first = p;
    const unsigned char *last = first;
    while (*p) {
        if (!isspace(*p++)) {
            last = p;
        }
    }

I think that's a little easier to follow, since we now only have 2 levels of control statements (while+if) rather than the 3 previously (for+if+if).

It also gives another idea - perhaps we could decompose into two separate functions?

/* Returns a pointer to the first non-whitespace character in 'p'
   (which might be the terminating null).
   Return value should be considered const if input is actually const.
 */
char *trimmed_left(const char *input)
{
    if (!input) { return (char*)input; }
    const unsigned char *p = (const unsigned char *)input;
    while (isspace(*p)) {
        ++p;
    }
    return (char*)p;
}

/* Returns the length of string up to the last non-whitespace character
   (0 if no such characters are present).
 */
size_t trimmed_right_len(const char *const input)
{
    if (!input) { return 0; }
    const unsigned char *start = (const unsigned char *)input;
    const unsigned char *p = start;
    size_t len = 0;
    while (*p) {
        if (!isspace(*p++)) {
            len = (size_t)(p - start);
        }
    }
    return len;
}

char *trimmed_bound(const char *s, size_t *trimmed_size)
{
    size_t dummy_len;
    if (!trimmed_size) {
        trimmed_size = &dummy_len;
    }
    char *result = trimmed_left(s);
    *trimmed_size = trimmed_right_len(result);
    return result;
}

Although that's more code, the parts can now be tested separately, and callers can make different choices about how to compose them.


Full code

With no need for the non-standard strdup() and no functions with error returns to check.

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

/* Returns a pointer to the first non-whitespace character in 'p'
   (which might be the terminating null).
   Return value should be considered const if input is actually const.
 */
char *trimmed_left(const char *input)
{
    if (!input) { return (char*)input; }
    const unsigned char *p = (const unsigned char *)input;
    while (isspace(*p)) {
        ++p;
    }
    return (char*)p;
}

/* Returns the length of string up to the last non-whitespace character
   (0 if no such characters are present).
 */
size_t trimmed_right_len(const char *const input)
{
    if (!input) { return 0; }
    const unsigned char *start = (const unsigned char *)input;
    const unsigned char *p = start;
    size_t len = 0;
    while (*p) {
        if (!isspace(*p++)) {
            len = (size_t)(p - start);
        }
    }
    return len;
}

/* Returns a pointer to the first non-whitespace character in 'p'.
   If 'trimmed_size' is not null, writes the result length there.
 */
char *trimmed_bound(const char *s, size_t *trimmed_size)
{
    size_t dummy_len;
    if (!trimmed_size) {
        trimmed_size = &dummy_len;
    }
    char *result = trimmed_left(s);
    *trimmed_size = trimmed_right_len(result);
    return result;
}

char *trimmed_string(char *input)
{
    if (!input) { return input; }
    char *s = trimmed_left(input);
    s[trimmed_right_len(s)] = '\0';
    return s;
}
#include <stdio.h>
#include <string.h>

/* return 0 for success, 1 for failure */
static int test_trim(const char *file, int line,
                     const char *input, const char *expected)
{
    size_t len;
    const char *s = trimmed_bound(input, &len);
    if (len == strlen(expected) && memcmp(s, expected, len) == 0) {
        /* test passed */
        return 0;
    }
    fprintf(stderr, "%s:%d: trim('%s') should yield '%s' but got '%.*s'\n",
            file, line, input, expected, (int)len, s);
    return 1;
}

#define TEST_TRIM(input, expected) test_trim(__FILE__, __LINE__, input, expected)

int main(void)
{
    return TEST_TRIM("", "")
        |  TEST_TRIM(" ", "")
        |  TEST_TRIM("  ", "")
        |  TEST_TRIM("\t\n", "")
        |  TEST_TRIM("foo bar", "foo bar")
        |  TEST_TRIM(" foo bar", "foo bar")
        |  TEST_TRIM("  foo bar", "foo bar")
        |  TEST_TRIM("  bar  foo  ", "bar  foo")
        |  TEST_TRIM("foo bar  ", "foo bar")
        |  TEST_TRIM("\n foo \t bar \r", "foo \t bar");
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Nice trimmed_bound(). UV. Yet for similar reasons that char *strchr(const char *s, int c); and other STL functions return char *, I'd expect the same here. We could go the _Generic route, but that is "work". \$\endgroup\$ Feb 15, 2023 at 14:12
  • 1
    \$\begingroup\$ Spot on: brain in string mode - well, that and evolving from the earlier iteration that used strcmp(). memcmp() is probably clearer about what's tested. \$\endgroup\$ Feb 15, 2023 at 14:24
  • \$\begingroup\$ I never liked the strchr interface - it's risky, and I've not seen a compiler option to warn when it's misused (or to provide custom attribute to mark user functions that propagate const). But certainly something to consider, for familiarity reasons. \$\endgroup\$ Feb 15, 2023 at 14:38
3
\$\begingroup\$

This works, but feels convoluted, is there a cleaner of doing it?

If you are fine with a bit of inefficiency, and with keeping the original buffer (which may be oversized for the new string), then an in-place trim could be as simple as two loops. One for finding the "actual" start of the string (so, skipping spaces at the beginning), and another one moving the string inside itself, also tracking the last non-space character encountered:

void string_trim(char *input) {
    char *dest=input;
    while(*input==' ') {
        input++;
    }
    char *end=input;
    while(*input) {
        if((*dest++=*input++)!=' ') {  // this line uses that x=y also has a value
            end=dest;                  // this line uses that dest is incremented already
        }
    }
    *end=0;
}

The inefficiencies here are that the "move" always happens, even when there are no spaces at the start, and spaces at the end are also moved before getting thrown away. And the deficiency is that the string is just sitting in its original location, which is most probably a bit oversized now.
https://ideone.com/iocf8W


Also, reallocating pointer that was passed to a function and allocated elsewhere first - I'm suspecting this would not be considered good practice or am I wrong?

Well, one may rather expect a strxy()-like signature, like char *strdup(const char *src);, so whatever string goes in is a const, and won't be tampered with, and the trimmed string would be returned. So char *strtrim(const char *input);.

Then it still needs a complete scan, for finding the first and last non-spaces, and then allocate a suitable buffer for the trimmed string and make the copy:

char *strtrim(const char *input) {
    while(*input==' ') {
        input++;
    }
    const char *start=input;
    const char *end=input;
    while(*input) {
        if(*input!=' ') {       // yes, these 4 lines could be uglified as in the other
            end=input+1;        //
        }                       // if(*input++!=' ')
        input++;                //     end=input;
    }
    size_t len=end-start;
    char *result=malloc(len+1);
    if(result) {
        result[len]=0;
        memcpy(result,start,len);
    }
    return result;
}

https://ideone.com/lJUzV1

\$\endgroup\$
1
  • \$\begingroup\$ It is is highly unlikely strings are getting exactly sized memory allocation blocks anyway. Any loss is efficiency from non reallocating is dwarves by the advantages of not doing so. \$\endgroup\$ Feb 15, 2023 at 6:38
3
\$\begingroup\$

A single point here. I would choose between one of the following two design options:

  1. Either change in-place (and expect only one pointer); I'd prefer that because it's likely the most common use case (sanitize some crappy input and never look at the original again).
  2. Or produce a modified copy (and then expect an additional pointer for the result, like with memcpy()or strcpy()).

In both cases the memory management is left to the caller, for good reason: Either one or none of the source and target may be dynamically allocated, so that you can neither be sure that you can free nor that you should overwrite them. With short strings a dynamic allocation may dominate the run time behavior which may be unacceptable in performance critical code.

strdup() is in my opinion somewhat of an oddity. None of the other string or memory functions implicitly allocates new memory. If you use strdup() in code that otherwise works with local or static strings you complicate your memory management on the caller side (some of your strings now are dynamically allocated and must be freed, others not, and you must track that).

Certainly changing the original pointer would be an oddity except in very specialized circumstances.

\$\endgroup\$
2
\$\begingroup\$

This code will definitely fail on an all-space string.

Also, there is no need to increase and decrease the pointer with easy-to-make-it-wrong calculations. If you want to keep the original value - you could simply write

while (*(input_copy + start_index) == ' ')
{
    ++start_index;
}

Or - even better - add an extra pointer pointing to the beginning (and to the end).

\$\endgroup\$
2
\$\begingroup\$

There are several other reviews, all giving good advice.

I'll add a further recommendation, which is to learn what's provided in the standard library, and make good use of it. Consider this fragment:

  int start_index = 0;
  while (*(input_copy++) == ' ') {
    start_index++;
  }

Instead of writing this loop by hand, we can use strspn() to count the number of leading spaces:

    const size_t start_index = strspn(start_index, " ");

An incidental advantage is that this change allows us to adjust the set of characters we consider to be whitespace (e.g. tabs, newlines, page-break, etc.) just by changing the second argument.

There's not a ready-made function for the corresponding operation working from the end of the string, so a hand-crafted loop is still required there.


Similarly, this code can be replaced by memcpy() (and probably will be, by a good optimising compiler):

  int index = 0;
  for (int i = start_index; i < end_index; i++) {
    (*input)[index] = input_copy[i];
    index++;
  }

Anyone needing to modify your code will find it much easier to understand if it uses library functions rather than rolling your own (the first reaction is often to spend time scrutinising it to see what it does that's not provided by the standard function).


A char is always 1 char in size, so multiplying a quantity by sizeof (char) achieves nothing except to clutter the code. Just use the size directly.

OTOH, if we write a wide-char version we should multiply length including terminator by the character size: sizeof (wchar_t) * (new_length + 1).

\$\endgroup\$

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.