Slice function for C

Question

I am trying to create a non-generic slice function. I want a 'view' not a copy.

The scan_from arg is so that successive calls know where to start from:

const char *slice(const char *s,
                  const size_t i,
                  size_t *scan_from) {
  const size_t substr_length = i - *scan_from + 1;
  if (substr_length > 0) {
    char *substr = (char*)malloc(sizeof(char) * substr_length);
    if (substr == NULL) exit(ENOMEM);
    assert(substr_length == snprintf(
                              substr, substr_length + 1,
                              "%.*s", (int)substr_length,
                              s + *scan_from);
    *scan_from = substr_length + i;
    return substr;
  }
  return NULL;
}

Use is for parsing a large const char* and labelling/tagging each piece (minimum 1 char), by iterating over each char and seeing if the 'word' is finished, and if so slice-ing it out, otherwise keep iterating (i) until 'word' is finished or end of const char* is reached.

Answer 1

We're missing the necessary #include lines for malloc(), exit(), assert() and snprintf():

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

The argument names s and i don't convey much information - I find it hard to see how this function is to be used (and the lack of unit tests means there's no examples to consult).

We could make all the arguments const, I think:

const char *slice(const char *const s,
                  const size_t i,
                  size_t *const scan_from)

Don't cast the result of malloc(), and don't multiply by 1. But do fix the bug of not allocating enough space to write the terminating null character:

char *substr = malloc(substr_length + 1);

It's good that you check the return value from malloc - but it's probably better to return a null pointer (after setting errno = ENOMEM) than to exit the program when allocation fails.

The use of snprintf() seems to me to be a very long-winded way to write strncpy(substr, s + *scan_from, substr_length); substr[substr_length] = '\0';. The assert() doesn't appear to be justified, as there's nothing that guarantees that there are i or more characters remaining in the string. Also, we have functional code within the assert() macro invocation - that means that production builds won't call snprintf() at all - another bug.

Answer 2

Following from @Toby Speight good review.

Goal unclear

OP has "I want a 'view' not a copy.", yet uses malloc to make a copy.

Lacking documentation

Poor for code to have a buried memory allocation yet does not advise that the caller should free it.

"to create a non-generic slice function. I want ....", "The scan_from arg is ... " and "Use is for ..." deserve to be in code as a comment and/or code should be clear enough to explain itself.

The parameters names s, i in const char *slice(const char *s, const size_t i, size_t *scan_from) are not informative enough.

Document at a high level as if caller has access to the function declaration and not the definition. (in the .h file)

Array or string?

It appears that the function is to slice an array of char. Yet "%.*s" terminates early if a null character is found. This is a bug for an array function.

A helper function that exits!?

exit(ENOMEM) is surprising and undocumented in function description. @Mr R Further, ENOMEM is not part of the standard C lib and reduces portability.

// if (substr == NULL) exit(ENOMEM);
if (substr == NULL) return NULL;

Avoid function change in non-debug mode

Do not bury important functionality in an assert().

// assert(substr_length == snprintf(substr, substr_length + 1,
//    "%.*s", (int)substr_length, s + *scan_from);

int len = snprintf(substr, substr_length + 1, "%.*s", 
    (int)substr_length, s + *scan_from);
assert(len >= 0 && (unsigned) len == substr_length);

Formatting

Use an auto formatter.

Allocate to the referenced object

Easier to code right, review and maintain. Cast not needed.

// char *substr = (char*)malloc(sizeof(char) * substr_length);
char *substr = malloc(sizeof substr[0] * substr_length);

Be prepared for i < *scan_from

Recall size_t is an unsigned type and i - *scan_from can become a large value near SIZE_MAX.

Robust code parameter validation takes into account i, *scan_from could independently be any value [0 ... SIZE_MAX].

// const size_t substr_length = i - *scan_from + 1;
// if (substr_length > 0) {

if (i >= *scan_from) {
  const size_t substr_length = i - *scan_from + 1;

const return?

I'd expect a return types char * and not const char * as code is providing a allocated buffer for caller's use.

Bug

In extreme use (int)substr_length fails when substr_length > INT_MAX. Function description would mention that. Better code would work for all substr_length. Consider memcpy() instead for an array function.