Is this a good substrate for C?
See also C-tokenizer
Here is a quick substr () for C I wrote (yes, variable initializations need to be moved to the beginning of a function, etc., but you get the idea)
I've seen a lot of smart substr () implementations that are simple one-line calls to strncpy ()!
They are all wrong (strncpy does not guarantee null termination, and hence the call may NOT invoke the correct substring!)
What could be better?
Extract the errors!
char* substr(const char* text, int nStartingPos, int nRun)
{
char* emptyString = strdup(""); /* C'mon! This cannot fail */
if(text == NULL) return emptyString;
int textLen = strlen(text);
--nStartingPos;
if((nStartingPos < 0) || (nRun <= 0) || (textLen == 0) || (textLen < nStartingPos)) return emptyString;
char* returnString = (char *)calloc((1 + nRun), sizeof(char));
if(returnString == NULL) return emptyString;
strncat(returnString, (nStartingPos + text), nRun);
/* We do not need emptyString anymore from this point onwards */
free(emptyString);
emptyString = NULL;
return returnString;
}
int main()
{
const char *text = "-2--4--6-7-8-9-10-11-";
char *p = substr(text, -1, 2);
printf("[*]'%s' (\")\n", ((p == NULL) ? "<NULL>" : p));
free(p);
p = substr(text, 1, 2);
printf("[*]'%s' (-2)\n", ((p == NULL) ? "<NULL>" : p));
free(p);
p = substr(text, 3, 2);
printf("[*]'%s' (--)\n", ((p == NULL) ? "<NULL>" : p));
free(p);
p = substr(text, 16, 2);
printf("[*]'%s' (10)\n", ((p == NULL) ? "<NULL>" : p));
free(p);
p = substr(text, 16, 20);
printf("[*]'%s' (10-11-)\n", ((p == NULL) ? "<NULL>" : p));
free(p);
p = substr(text, 100, 2);
printf("[*]'%s' (\")\n", ((p == NULL) ? "<NULL>" : p));
free(p);
p = substr(text, 1, 0);
printf("[*]'%s' (\")\n", ((p == NULL) ? "<NULL>" : p));
free(p);
return 0;
}
Exit:
[*]'' (")
[*]'-2' (-2)
[*]'--' (--)
[*]'10' (10)
[*]'10-11-' (10-11-)
[*]'' (")
[*]'' (")
a source to share
I would say return NULL
if the input is invalid, not an empty string malloc()
ed. This way, you can check if the function with failed or not if(p)
, not if(*p == 0)
.
Also, I think your function is wasting memory because emptyString
- it is only free()
d in one conditional expression. You must make sure that you do free()
it unconditionally, i.e. Right in front return
.
Regarding your comment on strncpy()
non-NUL line completion (that's true), if you use calloc()
to allocate a line instead of malloc()
, it won't be a problem if you allocate one more byte than you copy, since it calloc()
automatically sets all values (including, in this case, end) to 0.
I would give you more notes, but I don't like reading camelCase. Not that there is anything wrong with that.
EDIT: Regarding your updates:
Remember, the C standard defines it sizeof(char)
as 1 regardless of your system. If you are using a computer that uses 9 bits per byte (God forbid) it sizeof(char)
will still be 1. Not that there is anything wrong with the expression sizeof(char)
- it clearly shows your intent and ensures symmetry to calls to calloc()
or malloc()
for other types. But sizeof(int)
really useful ( int
can be different sizes on 16- and 32- and these newfangled 64-bit computers). The more you know.
I would also like to reiterate that consistency with most other C code is to return NULL
to error, not to ""
. I know that many functions (for example strcmp()
) are likely to do bad things if you pass NULL to them - that's to be expected. But the C Standard Library (and many other C APIs) takes the "Caller's responsibility to validate NULL
, not responsibility to his / her role if he / she doesn't want to" approach . If you want to do it the other way, that's cool, but it goes against one of the strongest trends in C interface design.
Also, I would use strncpy()
(or memcpy()
) rather than strncat()
. Using strncat()
(s strcat()
) hides your intent - it makes someone look at your code, think what you want to add to the end of the line (which you do because after the calloc()
end is the beginning) when what you want to do asks string. strncat()
pretends that you are adding a line while strcpy()
(or some other copying procedure) will make it look more like what you intend to do. The next three lines all do the same in this context - pick the one that you think looks prettier:
strncat(returnString, text + nStartingPos, nRun);
strncpy(returnString, text + nStartingPos, nRun);
memcpy(returnString, text + nStartingPos, nRun);
Plus, strncpy()
and memcpy()
will likely be (slightly smaller) bits faster / more efficient than strncat()
.
text + nStartingPos
is the same as nStartingPos + text
. I put char *
it first as I think clearer, but whatever ordering you want to add is up to you. Also, parentheses around them are unnecessary (but nice) as they +
have higher precedence than ,
.
EDIT 2: The three lines of code don't do the same, but in this context they will all produce the same result. Thanks for catching me on this.
a source to share
Your function seems very complex for what should be a simple operation. Some problems (not all of them are bugs):
-
strdup()
and other memory allocation functions can fail, you must resolve any possible problems. - allocates resources (memory in this case) if and when you need it.
- you should be able to distinguish between bugs and actual bites. At this point, you don't know if the
malloc()
crash has occurred or if thesubstr ("xxx",1,1)
workersubstr ("xxx",1,0)
creates an empty string. - you don't need
calloc()
memory which you overwrite anyway. - all invalid parameters should either throw an error or be coerced into a valid parameter (and your API should document that).
- you don't need to set the local empty string to NULL after freeing it - it will be lost when the function returns.
- you don't need to use usr
strncat()
- you need to know the sizes and memory you have before doing any copying in order to use (most likely) fastermemcpy()
. - you are using base-1 and not base-0 for line offsets, goes against grain C.
The next segment is what I would do (I prefer the Python idiom of negative values for counting from the end of the string better, but I kept the length, not the end position).
char *substr (const char *inpStr, int startPos, int strLen) {
/* Cannot do anything with NULL. */
if (inpStr == NULL) return NULL;
/* All negative positions to go from end, and cannot
start before start of string, force to start. */
if (startPos < 0)
startPos = strlen (inpStr) + startPos;
if (startPos < 0)
startPos = 0;
/* Force negative lengths to zero and cannot
start after end of string, force to end. */
if (strLen < 0)
strLen = 0;
if (startPos >strlen (inpStr))
startPos = strlen (inpStr);
/* Adjust length if source string too short. */
if (strLen > strlen (&inpStr[startPos]))
strLen = strlen (&inpStr[startPos]);
/* Get long enough string from heap, return NULL if no go. */
if ((buff = malloc (strLen + 1)) == NULL)
return NULL;
/* Transfer string section and return it. */
memcpy (buff, &(inpStr[startPos]), strLen);
buff[strLen] = '\0';
return buff;
}
a source to share
You can also use the memmove function to return a substring from start to length. Improving / adding another solution from paxdiablo's solution:
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
char *splitstr(char *idata, int start, int slen) {
char ret[150];
if(slen == NULL) {
slen=strlen(idata)-start;
}
memmove (ret,idata+start,slen);
return ret;
}
/*
Usage:
char ostr[]="Hello World!";
char *ores=splitstr(ostr, 0, 5);
Outputs:
Hello
*/
Hope this helps. Tested on Windows 7 Home Premium with TCC C Compilier.
a source to share