code review

[nullptr]

Hello group,

I am learning C using K&R2 book and as an exercise wrote grep-like
program:

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

#define MAXLEN 100

void stdinput(char **);

char line[MAXLEN];
int lineno = 0, i = 0;


int main(int argc, char *argv[])
{

if(argc > 1)
{
if(((strcmp(argv[1], "-l")) == 0 ||
(strcmp(argv[1], "--line-number")) == 0 ))
i = 1;

if(argc >= 3 && !( i && argc == 3))
{
FILE *fp;
char **p;

if(i)
p = &argv[3];
else
p = &argv[2];

if(*p[0] == '-')
stdinput(&argv[2]), exit(EXIT_SUCCESS);

while(*p)
{
if(( fp = fopen(*p, "r")) != NULL)
{
while((fgets(line, MAXLEN, fp)) != NULL) {
if(i)
{
if(strstr(line, argv[2]) != NULL)
{
printf("%s:%d: ", *p, lineno);
printf("%s", line);
}
}
else
if(strstr(line, argv[1]) != NULL)
printf("%s", line);
lineno++;
}
fclose(fp);
}
else
fprintf(stderr, "Can't open filename %s.\n", *p);
++p;
}

}

else
stdinput(&argv[2]);
}

else
printf("grep v0.2\n"
"Usage: %s [OPTION]... [PATTERN] [FILE]\n" "Search the stdin/FILE
for the occurrence(s) of the PATTERN\n" "\nAvailable options:\n"
" -l, --line-number print the line number for the
matching lines\n", *argv);
return 0;
}

void stdinput(char **p)
{

while( (fgets(line, MAXLEN, stdin)) != NULL) {
lineno++;
if(i)
{
if(strstr(line, *p) != NULL)
{
printf("%d: ", lineno);
printf("%s", line);
}
}
else
if(strstr(line, *p-1) != NULL)
printf("%s", line);
}
}

Any suggestions, correction, comments appreciated. --
nullptr

 

[Malcolm]

I am learning C using K&R2 book and as an exercise wrote grep-like
program:

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

#define MAXLEN 100

That's a bit of a mean max line length.

void stdinput(char **);

char line[MAXLEN];
int lineno = 0, i = 0;

These should not be global. Particularly you don't want a global variable
named "i".

int main(int argc, char *argv[])
{

if(argc > 1)
{
if(((strcmp(argv[1], "-l")) == 0 ||
(strcmp(argv[1], "--line-number")) == 0 ))
i = 1;

You need a comment here. It's very difficult to tell if the argc and argv
manipulations are correct or not,

if(argc >= 3 && !( i && argc == 3))
{
FILE *fp;
char **p;

if(i)
p = &argv[3];
else
p = &argv[2];

if(*p[0] == '-')
stdinput(&argv[2]), exit(EXIT_SUCCESS);

Comma operators are hardly ever used in C. Another programmer will be
puzzled to find it here and wonder what is going on.

while(*p)
{
if(( fp = fopen(*p, "r")) != NULL)
{
while((fgets(line, MAXLEN, fp)) != NULL) {
if(i)
{
if(strstr(line, argv[2]) != NULL)
{
printf("%s:%d: ", *p, lineno);
printf("%s", line);
}
}
else
if(strstr(line, argv[1]) != NULL)
printf("%s", line);
lineno++;
}
fclose(fp);
}
else
fprintf(stderr, "Can't open filename %s.\n", *p);
++p;
}

}

else
stdinput(&argv[2]);
}

else
printf("grep v0.2\n"
"Usage: %s [OPTION]... [PATTERN] [FILE]\n" "Search the stdin/FILE
for the occurrence(s) of the PATTERN\n" "\nAvailable options:\n"
" -l, --line-number print the line number for the
matching lines\n", *argv);
return 0;
}

You need a comment here. What is the function intended to do? What is the
parameter p ?
Are you sure you really need to handle stdin as a special case? Can you not
write a dogrep(FILE *fp, const char *pattern, int linenums) function?

void stdinput(char **p)
{

while( (fgets(line, MAXLEN, stdin)) != NULL) {
lineno++;
if(i)
{
if(strstr(line, *p) != NULL)
{
printf("%d: ", lineno);
printf("%s", line);
}
}
else
if(strstr(line, *p-1) != NULL)
printf("%s", line);
}
}

Any suggestions, correction, comments appreciated. --
nullptr

The main weakness I detected was the use of globals.

fgets() isn't actually all that useful a function, since you need to know
what to do if the buffer overflows - that is, it contains no newline
character and the file isn't ended.
One solution is to bump up the line length to, say, 1000 characters, and say
that any line longer than that is obviously corrupted, so terminate the
program.

Another more elegant solution is to say that any over-long line is corrupt,
and discard the remaining portion of the line in the input buffer. This
means that the program will continue running, which has the disadvantage
that if it is passed a garbage file, say a binary, it will continue to spew
garbage results.

Another solution is to write a char *getline(FILE *fp) function which hold a
line of any length in a dynamically-allocated buffer. Of course you still
need to check the return value for out-of-memory.

 

[nullptr]

The reason why I used global variables and the separate stdinput()
function is because didn't want to use goto's in cases like if(*p[0] ==
'-') where if filename is '-', the program starts reading from stdin.

I could have avoided globals by using long argument lists but it didn't
seem like a good solution for me.

 

[nullptr]

fgets() isn't actually all that useful a function, since you need to
know what to do if the buffer overflows - that is, it contains no
newline character and the file isn't ended.
One solution is to bump up the line length to, say, 1000 characters, and
say that any line longer than that is obviously corrupted, so terminate
the program.

Well, line can't be longer that MAXLINE, because fgets will read at most
MAXLINE-1 characters. The rest of the line will remain in the buffer
untill the next read.

Anyway, here is my modified code:

#include <stdio.h>
#include <string.h>

#define MAXLEN 1000

int main(int argc, char *argv[])
{
char line[MAXLEN];
int lineno = 0, i = 0;

if(argc > 1)
{

/* check for the first parameter */

if(((strcmp(argv[1], "-l")) == 0 ||
(strcmp(argv[1], "--line-number")) == 0 ))
i = 1;

if(argc >= 3 && !( i && argc == 3)
&& strcmp(argv[argc-1], "-") != 0)
{
FILE *fp;
char **p;

if(i)
p = &argv[3]; /* filename after OPTIONS and PATTERN */
else
p = &argv[2]; /* filename after PATTERN */

while(*p)
{
if(( fp = fopen(*p, "r")) != NULL)
{
while((fgets(line, MAXLEN, fp)) != NULL) {
if(i)
{
if(strstr(line, argv[2]) != NULL)
{
printf("%s:%d: ", *p, lineno);
printf("%s", line);
}
}
else
if(strstr(line, argv[1]) != NULL)
printf("%s", line);
lineno++;
}
fclose(fp);
}
else
fprintf(stderr, "Can't open filename %s.\n", *p);
++p;
}

}

else
while( (fgets(line, MAXLEN, stdin)) != NULL) {
lineno++;
if(i)
{
if(strstr(line, argv[2]) != NULL)
{
printf("%d: ", lineno);
printf("%s", line);
}
}
else
{
if(strstr(line, argv[1]) != NULL)
printf("%s", line);
}
}
}

else
printf("grep v0.2\n"
"Usage: %s [OPTION]... [PATTERN] [FILE]\n" "Search the stdin/FILE
for the occurrence(s) of the PATTERN\n" "\nAvailable options:\n"
" -l, --line-number print the line number for the
matching lines\n", *argv);
return 0;
}

Thanks,

 

[Mac]

Anyway, here is my modified code:

#include <stdio.h>
#include <string.h>

#define MAXLEN 1000

int main(int argc, char *argv[])
{
char line[MAXLEN];
int lineno = 0, i = 0;

if(argc > 1)
{

/* check for the first parameter */

if(((strcmp(argv[1], "-l")) == 0 ||
(strcmp(argv[1], "--line-number")) == 0 ))
i = 1;

if(argc >= 3 && !( i && argc == 3)
&& strcmp(argv[argc-1], "-") != 0)
{
FILE *fp;
char **p;

if(i)
p = &argv[3]; /* filename after OPTIONS and PATTERN */
else
p = &argv[2]; /* filename after PATTERN */

while(*p)

This isn't a portable way to find the last argument. Dereferencing
argv[argc] is UB. You should check that the index value is always
less than argc. The same applies to the usage of *p that follows.

Hmm. It's true that dereferencing argv[argc] (i.e., *(argv[argc]))is
undefined behavior since argv[argc] is always NULL. But the OP isn't
dereferencing it. In effect, in the case where p gets the value of
&argv[3], p is pointing to the same place as (argv+3), and that means that
*p == NULL.

I'm not sure what happens when you pass a NULL to fopen as a filename,
however.

[snip]


Mac
--

 

[Mac]

Anyway, here is my modified code:

#include <stdio.h>
#include <string.h>

#define MAXLEN 1000

int main(int argc, char *argv[])
{
char line[MAXLEN];
int lineno = 0, i = 0;

if(argc > 1)
{

/* check for the first parameter */

if(((strcmp(argv[1], "-l")) == 0 ||
(strcmp(argv[1], "--line-number")) == 0 ))
i = 1;

if(argc >= 3 && !( i && argc == 3)
&& strcmp(argv[argc-1], "-") != 0)
{
FILE *fp;
char **p;

if(i)
p = &argv[3]; /* filename after OPTIONS and PATTERN */
else
p = &argv[2]; /* filename after PATTERN */

while(*p)

This isn't a portable way to find the last argument. Dereferencing
argv[argc] is UB. You should check that the index value is always
less than argc. The same applies to the usage of *p that follows.

Hmm. It's true that dereferencing argv[argc] (i.e., *(argv[argc]))is
undefined behavior since argv[argc] is always NULL. But the OP isn't
dereferencing it. In effect, in the case where p gets the value of
&argv[3], p is pointing to the same place as (argv+3), and that means that
*p == NULL.

Ooops. The OP did indeed dereference argv[argc], but Nick snipped it. My
apologies.

[snip]

Mac
--

 

[nullptr]

On Sat, 23 Aug 2003 19:02:14 +0000, Mac wrote: Ooops. The OP did indeed
dereference argv[argc], but Nick snipped it. My apologies.

[snip]

Mac

No, I'm not dereferencing argv[argc], because (argc == 3 && i) is never
true in this part of the code, as pointed by the if test: !( i && argc ==
3). Therefore if i == 1, argc will need to be larger that 3...

I'm going to try to put the pattern matching code into the separate
function though, and post the results later.

Thanks for your suggestions, they helped me a lot.