Skip to main content
deleted 647 characters in body
Source Link
S.S. Anne
  • 1.7k
  • 8
  • 27

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Instead of using assert in your functions, just rely on the user to pass in a non-null pointer.

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

All these (unsigned int) casts are not necessary:

pVer->major = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->minor = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->build = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->revision = (unsigned int) atoi(token);

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.

The main thing that I'd like to say is:

Don't reinvent the wheel.

If you have strverscmp available (GNU libc and others), and your code doesn't have to be portable, why not use it?

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Instead of using assert in your functions, just rely on the user to pass in a non-null pointer.

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

All these (unsigned int) casts are not necessary:

pVer->major = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->minor = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->build = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->revision = (unsigned int) atoi(token);

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.

The main thing that I'd like to say is:

Don't reinvent the wheel.

If you have strverscmp available (GNU libc and others), and your code doesn't have to be portable, why not use it?

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.

added 196 characters in body
Source Link
S.S. Anne
  • 1.7k
  • 8
  • 27

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Instead of using assert in your functions, just rely on the user to pass in a non-null pointer.

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

All these (unsigned int) casts are not necessary:

pVer->major = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->minor = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->build = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->revision = (unsigned int) atoi(token);

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.

The main thing that I'd like to say is:

Don't reinvent the wheel.

If you have strverscmp available (GNU libc and others), and your code doesn't have to be portable, why not use it?

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Instead of using assert in your functions, just rely on the user to pass in a non-null pointer.

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

All these (unsigned int) casts are not necessary:

pVer->major = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->minor = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->build = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->revision = (unsigned int) atoi(token);

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Instead of using assert in your functions, just rely on the user to pass in a non-null pointer.

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

All these (unsigned int) casts are not necessary:

pVer->major = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->minor = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->build = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->revision = (unsigned int) atoi(token);

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.

The main thing that I'd like to say is:

Don't reinvent the wheel.

If you have strverscmp available (GNU libc and others), and your code doesn't have to be portable, why not use it?

Source Link
S.S. Anne
  • 1.7k
  • 8
  • 27

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Instead of using assert in your functions, just rely on the user to pass in a non-null pointer.

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

All these (unsigned int) casts are not necessary:

pVer->major = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->minor = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->build = (unsigned int) atoi(token);
token = strtok(NULL, delim);
pVer->revision = (unsigned int) atoi(token);

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.