1
\$\begingroup\$

A very simple function that can be used such that it returns a pointer to the first character on which func returned 0. However, it also handles escaped lines. Needless to say this could be very useful in my cases, for example skipping spaces when converting a csv to an array.

/** Skip characters from @src
* @func: isspace, isdigit, isalpha, etc can be used
*/
char *skip (const char *src, int(*func)(int))
{
    char* p = NULL;

    for(p = src; *p != '\0'; p++)
    {
        if(!func(*p))
        {
            if(*p == '\\')
            {
                p++;

                if(*p == '\r') /* Will probably have to deal with annoying MS-style line endings when the source string is loaded from a file */
                {
                    p++;
                }
                if(*p == '\n')
                {
                    continue;
                }
            }

            break;
        }
    }

    return p;
}

Example test usage:

int ishspace (char ch)  { return(ch == ' ' || ch == '\t'); }

int main (void)
{
    char* p = skip(" \t \\\n \t Hello World\n", ishspace);

    printf("%s", p);

    return 0;
}

Output:

> Hello World

>

\$\endgroup\$
3
  • \$\begingroup\$ What should happen if func returns true on '\\', like ispunct? \$\endgroup\$ Aug 11 '21 at 4:35
  • \$\begingroup\$ @PavloSlavynskyy then the loop continues (the `\` is consumed) as it is supposed to be? If the line is escaped though, I imagine it will break on the NL \$\endgroup\$
    – Edenia
    Aug 11 '21 at 4:46
  • \$\begingroup\$ That should be easily solved by moving the inner condition toplevel and adding "else". Do you think I should edit it or wait for a complete review. Hopefully there are better, potentially faster ways to achieve this, as it is often used a lot and in performance-sensitive context. \$\endgroup\$
    – Edenia
    Aug 11 '21 at 4:52
3
\$\begingroup\$

Code Review

Problem #1

In the function header char *skip (const char *src, int(*func)(int)), in particular, making the argument char *src a const does not make much sense, as it is not regarded as so throughout the function.

For instance, p is assigned src in the for loop, despite the fact that p is not a pointer to a const char. p is also returned from the function, despite the fact that the function does not return a const char (it is just char).

Hence, you should either

  1. Drop the const from const char *src or
  2. Make the function return a const char and make p a const char as well (if you are doing this, you must also make char* p a const char* p in the main function)

Problem #2

Also in the function header char *skip (const char *src, int(*func)(int)), in particular, making the argument int(*func)(int) does not make sense. At least, according to your "test usage".

For instance, you have defined your ishspace function as int ishspace (char ch). I do not know if this was intentional or not, but this translates to int (*)(char) (because it accepts char as an argument), not int (*)(int) as your skip function requires. For your information, isspace(), isdigit(), isalpha(), etc. (the ones that are defined in <ctype.h>, I assume) all accept int as an argument.

Hence, you should either

  1. Change the argument from int(*func)(int) to int(*func)(char); though, this is probably not what you want

  2. Change the ishspace function in the usage example to accept int instead of char (this will not affect the overall capability of the function anyways)

This can be improved. Using something like a switch statement for comparing the values of *p would also be more readable/efficient, if you will have to compare against 6 or more escape sequences in the future.

"Parting thoughts"

In terms of design choices, I am personally against passing isspace(), isdigit(), isalpha(), etc. as an argument of int(*func)(int). Instead, it may be better to actually just use some those functions within the skip function, or try to create separate functions that checks separate things. This is due to a couple of reasons:

  1. It gives the users (programmers) too much power by allowing them to pass in virtually whatever function they want. This could generate all sorts of undefined behavior and even security issues.
  2. It can conflict with your internal checks for certain escape sequences, like '\\' in particular. For instance, ispunct() function (assuming that we are talking about the functions in <ctype.h>) will return non-zero for '\\', which is definitely not what you want.
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Your "readability" improvement changes the logic . OP's only advances p on an \r following a '\\'. Yours advances on all \rs. \$\endgroup\$
    – AShelly
    Aug 11 '21 at 23:10
  • \$\begingroup\$ I don't believe you can give programmers "too much power". You just need to document what happens with the power you give them. If application end users were able to pass in arbitrary functions, that would be a different story. \$\endgroup\$
    – AShelly
    Aug 11 '21 at 23:13
  • \$\begingroup\$ @AShelly Edited accordingly. I did not see that for the if-statement. As for giving programmers "too much power", I think my Problem #2 demonstrates exactly what you are talking about where "end users were able to pass in arbitrary functions", despite the said documentation. \$\endgroup\$
    – NghtCtrL
    Aug 12 '21 at 2:09
  • \$\begingroup\$ I commented that 2. was basically a missed design consideration. Instead of editing the question, I just left it as-is in case someone already started a C/R. I didn't paid attention for the ishspace accepting char too, that's definitely not correct. I only have to agree with @AShelly that instead of purposely restricting programmers, I should just carefully document the function. \$\endgroup\$
    – Edenia
    Aug 12 '21 at 15:27
1
\$\begingroup\$

On the function arguments: src should definitely be a const char*, that is a signal to the caller that this function guarantees not to modify the string. Enforce that guarantee by making p a const char* also.

And that guarantee implies that you should then return an const char*, so that this function is not a backdoor for the caller to cast away constness.

I would probably rename func to testfunc or predicate.

I don't think there is any problem with accepting an arbitrary test function. Tons of code out there uses callbacks. This isn't end-user facing code, and you are not responsible for limiting the "power" of programmers who use your code. You should clearly document the conditions under which this function is called, and what is done with it's output, so that programmers understand what's happening. For example

// calls 'testfunc' on every non-escaped character in 'str' until it returns zero.

testfunc should take a char argument. Yes, isalpha and the like take integers, but the spec says

[isalpha's] argument is an int, the value of which the application shall ensure is representable as an unsigned char ...

Since you have chars, and you are responsible for ensuring chars are passed to isalpha, I would change the type and write an adapter

int alphatest(char c) { return isalpha(c); }

I don't understand the goal of the escape skipping part. Right now you skip calling the test func on any character following a '\\', but only if testfunc doesn't match the \\. That seems odd.

I would probably simplify the spec to: "doesn't test any character after an \". Or maybe "never matches on escaped newlines", if that's what you intend. Either way, I'd refactor to

for(p = src; *p != '\0'; p++)
{
    p = skip_escapes(p);
    if (p == '\0' or func(*p) == 0)
    {
        break;
    }
}

Where skip_escapes returns p or the first character after a valid escape sequence starting at p, whatever you decide the escape sequence to be.

\$\endgroup\$
2
  • \$\begingroup\$ int alphatest(char c) { return isalpha(c); } is UB when c < 0 and not EOF. In that case "value of which the application shall ensure is representable as an unsigned char" was not met. \$\endgroup\$ Aug 11 '21 at 23:33
  • \$\begingroup\$ "Right now you skip calling the test func on any character following a '\\', but only if testfunc doesn't match the \\. That seems odd." Please, take a look at my comment on the answer above. Also The function only explicitly skips "\", "\\r" and "\\r\n", not every character following a '\\' \$\endgroup\$
    – Edenia
    Aug 12 '21 at 15: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.