strReadLine function (read line by line from a string)

Question

I'm new to C language and want to read a string line by line, I made this function and want to know what do you think about it, can I rely on it a production environment ?

/**
 * this function is not thread safe,
 */
char* strReadLine(char* text)
{
 static char* text2;
 static int start = 0;
 static int length = 0;
 int startTemp;
 int lineLength = 0;
 char* line = (char*) 0;
 startTemp = start;
 //not thread safe but at least make an error
 if(start != 0 && text2 != text){
 fprintf(stderr, "thread safe guerd fired by strReadLine function\n");
 exit(EXIT_FAILURE);
 }
 text2 = text;
 //thread safty end
 //if new string
 if(length == 0 || start == 0) length = strlen(text);
 if(!length) return (char*) 0;//the string is 0 length
 for(int x = startTemp; x < length; x++){
 start++;
 lineLength++;
 if( text[x] == '\n'){
 line = malloc(lineLength);//not lineLength+1 because I will remove the \n with 0円
 strncpy(line, text + (startTemp) , lineLength);
 line[lineLength - 1] = '0円';
 break;
 }
 }
 //if end of file && not the last line
 if(start == length && !line){
 start = 0;
 line = (char*) 0;
 }
 return line;
}

I have tested it with this text file

line1
line2
line3
line5
سطر6
line7

(line 4 is empty and line 6 contains non Latin characters stored in more than 1 byte in UTF-8 encoding)

like this, and it worked as expected

//char* input;this points to the string that is read from the file(I omitted the reading code)
char* line;
int x = 0;
while( (line = strReadLine(input)) ){
 x++;
 printf("%s\n",line);
 if(x == 300){printf("emergency break\n");break;}
}
x = 0;
while( (line = strReadLine(input)) ){
 x++;
 printf("%s\n",line);
 if(x == 300){printf("emergency break\n");break;}
}

Answer 1

Code format

Your use of spaces, braces and parantheses is unconventional in some places.

For example:

while( (line = strReadLine(input)) ){
 x++;
 printf("%s\n",line);
 if(x == 300){printf("emergency break\n");break;}
}

Conventionally this would look more like this:

while (line = strReadLine(input)) {
 x++;
 printf("%s\n", line);
 if (x == 300) {
 printf("emergency break\n");
 break;
 }
}

You could try using your IDE's auto-format feature and see how it arranges the code. Also you might want to have a look at some C coding conventions that can be found online, to see how people commonly format their C code in a readable way.

Thread safety

It seems like you are trying to guard the function against multiple threads executing it at the same time. However, the function's purpose is to read a string line by line, not to write to it. So as long as the function is only reading from its input, there should be no need to prevent multiple concurrent executions.

If multiple threads really did execute it at the same time, you could still get a race condition between checking the variables and overwriting it, and thus not get the error message when you would expect it.

Abusing booleanness

When checking whether an integer has the value 0, it is more readable if you compare it to 0, as you do in the first of the following two lines. It reads more intuitively, because a length cannot be true or false, as the second line implies, even though the compiler can handle both. Then you also wouldn't need the comment explaining what !length is supposed to mean. if (length == 0) says it explicitly.

if(length == 0 || start == 0) length = strlen(text);
if(!length) return (char*) 0;//the string is 0 length

• Accountant م: Thanks, regarding the thread safety, this function is not safe however it only reads, because of it's internal static variable start, when 2 threads are using it at the same time , the static variable start will be changing by the 2 threads in a wrong way, (e.g: first thread changes the start for him self, then the second thread use it and the function will start from the start of the string of the first thread), or I missed something ?
• Raimund Krämer: What I meant is that you might completely avoid that problem by not relying on the static variable, and instead adding another parameter for that, which you pass as a pointer or reference. Then every thread that might call that function would track its own state, rather than relying on some hidden state inside of the function, and you could remove the whole "thread safety" checking code altogether.