2
\$\begingroup\$

I wrote my first useful function in C which can be used in a different variety of situations as an alternative to common code logic.

segstr (originally named strseg but renamed to not use a reserved identifier) is a helper/utility function which can be used as an alternative to using strchr, strtok etc. for parsing information from a string. I originally wrote some code which used strchr to find the starting position of the next segment/token in a string but it quickly got repetitive when I need to use the same logic in other areas. This usually involves keeping track of certain information like the size and position of the current segment plus manually updating your pointer to point to the next segment.

This functions combines all that into a single function call which can be used in a loop. All you need is 2 variables to make this method of parsing work! The goal was to create a stateless and thread-safe function which does not depend on a internal-state and does not modify the supplied string (and preventing it from being used on read-only buffers). The supplied variables both act as the state and the required output from the function!

I have included a basic example of deconstructing a basic sentence separated by spaces in the main function.

The documentation is written to be compatible with Doxygen but I do not intend to actually use it to generate documentation.

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

/// @brief Get delimiter-separated segments from a string in a non-destructive method.
/// @details A useful function which can be used for parsing in a loop in a safe manner.
/// @author Damon Harris (TheDcoder@protonmail.com)
/// @param [in]     str        Current segment or start of the string.
/// @param [in,out] seg_len    Length of the segment current segment, value does not matter if seg_num is set to 0.
/// @param          delim      Character used for separation (delimiter).
/// @param [in,out] seg_num    Number of the current segment, set to 0 before first call.
/// @returns Pointer to the start of the next segment, seg_len is set to the length of the segment and seg_num is incremented by 1.
/// @remarks seg_num only has significance if its value is 0, the reason is for the function to be able to detect the initial call
///          so that it doesn't skip the first segment. seg_len can be any value on the first call since it is not used to calculate
///          the start of the next segment.
///
///          I was inspired by the lack of a helper function which I can use for parsing/tokenizing a string which is both stateless
///          and non-destructive (strtok_s is better than strtok but still modifies the supplied string). The best alternative was to
///          strchr and keep track of other stuff manually, this function tries to give out as much information as possible while keeping
///          the overall concept simple. The length and position (number) of the segment should be enough to get you started on parsing :)
char *segstr(const char *str, size_t *seg_len, char delim, unsigned int *seg_num) {
    if (*seg_num != 0) {
        // Check if we have reached the last segment
        str += *seg_len;
        if (str[0] == '\0') return NULL;

        // Proceed to the next segment
        ++str;
    }

    // Increment the segment number
    ++*seg_num;

    // Find the delimiter
    char *delim_start = strchr(str, delim);

    // Calculate the length of the segment
    if (delim_start) {
        // Difference between the delimiter and the start of the segment
        *seg_len = delim_start - str;
    } else {
        // We have reached the last segment
        // There is no delimiter, so use the remaining length
        *seg_len = strlen(str);
    }

    // Return the segment
    return str;
}

int main(void) {
    char *string = "This is sample text for testing in a sentence";

    char *segment;
    size_t segment_len;
    unsigned int segment_num = 0;
    segment = string;
    while (true) {
        segment = segstr(segment, &segment_len, ' ', &segment_num);

        if (!segment) {
            puts("Printed all of the words in the sentence!");
            return EXIT_SUCCESS;
        }

        printf("Word No. %u in sentence: %.*s\n", segment_num, segment_len, segment);
    }
}

Looking forward to all the comments and feedback on how I can improve the code and documentation :)

P.S I would like to thank several people from the ##c IRC channel at freenode for giving me an initial code review and very helpful suggestions.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Thanks for this good first question, and especially for providing a test program with it - I hope you get some good reviews, and I hope to see more of your contributions here in future! \$\endgroup\$ – Toby Speight Mar 1 at 10:55
  • \$\begingroup\$ @TobySpeight Thank you for the warm welcome! I will indeed try to put my future public contributions to code review :) \$\endgroup\$ – TheDcoder Mar 1 at 11:03
1
\$\begingroup\$

Valgrind reports a clean bill of health, but we get a handful of compiler warnings if we ask for them:

gcc -std=c17 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds      -Wconversion    214531.c    -o 214531
214531.c: In function ‘segstr’:
214531.c:42:20: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘long int’ may change the sign of the result [-Wsign-conversion]
         *seg_len = delim_start - str;
                    ^~~~~~~~~~~
214531.c:50:12: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
     return str;
            ^~~
214531.c: In function ‘main’:
214531.c:54:20: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
     char *string = "This is sample text for testing in a sentence";
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
214531.c:68:44: warning: field precision specifier ‘.*’ expects argument of type ‘int’, but argument 3 has type ‘size_t’ {aka ‘long unsigned int’} [-Wformat=]
         printf("Word No. %u in sentence: %.*s\n", segment_num, segment_len, segment);
                                          ~~^~                  ~~~~~~~~~~~

I think that the second one (discarding const) is inevitable given that we want to allow calling with const strings and non-const strings (C++ can solve this with overloading, but that's not possible in C, and so we have the same problem as strstr() and similar). It does mean that we need to be careful when we call segstr() that we ensure that we use the result consistently with the input.

It's probably better (particularly as this is intended for non-destructive reading) to use const char* as return type and require the caller to cast if writing is required - I think that's a fairly unlikely use-case.


It seems from reading the code that tokenising an empty string gives back a single empty segment, rather than no segments at all (i.e. we never return NULL when *seg_num is zero). I can see that either behaviour is reasonable, but the documentation should tell users what to expect.

Alternatively, if we return NULL, then there's no need for us to accept seg_num, and we can leave the counting (if required) to the caller. That way, callers that don't need the counting don't pay the cost of it (both the runtime cost and the cognitive overhead). As a compromise, we could interpret a null pointer passed as seg_num to indicate that counting isn't required.

const char *segstr(const char *str, size_t *seg_len, char delim, unsigned int *seg_num)
{
    if (seg_num) {
        // we're keeping count
        if (*seg_num != 0) {
            // Check if we have reached the last segment
            str += *seg_len;
            if (str[0] == '\0') return NULL;

            // Proceed to the next segment
            ++str;
        }

        // Increment the segment number
        ++*seg_num;

    } else {
        // user does any counting - interpret zero seg_len to mean "first segment"
        str += *seg_len;
        if (str[0] == '\0') return NULL;
        if (*seg_len != 0) {
            ++str;
        }
    }

    // Find the delimiter
    const char *delim_start = strchr(str, delim);

    // Calculate the length of the segment
    if (delim_start) {
        // Difference between the delimiter and the start of the segment
        *seg_len = (size_t)(delim_start - str);
    } else {
        // We have reached the last segment
        // There is no delimiter, so use the remaining length
        *seg_len = strlen(str);
    }

    // Return the segment
    return str;
}

Minor suggestions in the test program:

  • It's best to initialize segment as we declare it
  • The while loop doesn't need to be infinite; we should loop while segstr returns non-NULL, and move the if (!segment) block to follow the loop.
  • Let's make string constant.
int main(void)
{
    const char *const string = "This is sample text for testing in a sentence";

    const char *segment = string;
    size_t segment_len = 0;
    unsigned int segment_num = 0;

    while ((segment = segstr(segment, &segment_len, ' ', &segment_num))) {
        printf("Word No. %u in sentence: %.*s\n", segment_num, (int)segment_len, segment);
    }

    puts("Printed all of the words in the sentence!");
}

Alternative (doing our own counting):

    while ((segment = segstr(segment, &segment_len, ' ', NULL))) {
        printf("Word No. %u in sentence: %.*s\n", ++segment_num, (int)segment_len, segment);
    }

Trivial

  • typo in comment: has significance if it's value is: the correct word here is its.
\$\endgroup\$
  • \$\begingroup\$ Thanks for correcting the typo, I have already fixed it. For the first warning and all other warnings related to size_t to other integer conversions, I think we can ignore it as there is no undefined behavior. I prefer implicit conversions rather than explicit casting to satisfy the compiler's warnings. I agree with your advice about the const qualifier... Is there a way to write more detailed replies here? This comment box is too limiting and it also doesn't support multiple lines :( \$\endgroup\$ – TheDcoder Mar 1 at 15:24
  • \$\begingroup\$ Remember that the last warning (about arguments to printf()) won't implicitly get the necessary conversion (because it's an argument to a varargs function) and that may be UB (depending on your platform's integer types), so must be converted (e.g. by assignment to an int). Like you, I want to minimise conversions, but I differ in that a believe an explicit cast is better: it helps draw attention to something that requires extra checking/reasoning. \$\endgroup\$ – Toby Speight Mar 1 at 15:39
  • \$\begingroup\$ BTW, I'm not convinced I like my fudge of providing two different behaviours - I think it's confusing. But it might help you when you're considering the relative merits. \$\endgroup\$ – Toby Speight Mar 1 at 15:48
  • \$\begingroup\$ You are most correct about implicit conversion in printf, I should use an cast in that case. I agree that explicit casts draw attention but I have seen a lot of code where it starts making things look a lot more complex than they really are... I do not yet have the practical experience but I respect your preference. I will consider the behavior change as it does indeed look helpful and somewhat relates to the early prototype where the counting parameter was optional. By the way, can I implement the suggested improvements in this question and expect users to review it again? I am not familiar \$\endgroup\$ – TheDcoder Mar 1 at 16:59
  • \$\begingroup\$ I have decided to not change the behavior depending on seg_num after reconsidering, if the user wants to have 0 segments if an empty string is passed then they can implement it themselves :) \$\endgroup\$ – TheDcoder Mar 2 at 10:33

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.