4 ms·
Comments for your C learning: #include <stdbool.h> is better than typedef int bool; #define true 1 #define false 0 Always, always, surround
by leif 14y ago
Comments for your C learning:
#include <stdbool.h>
is better than
typedef int bool;
#define true 1
#define false 0
Always, always, surround your if and else clauses with brackets. There are plenty of ways leaving them naked will screw you over. This goes for any language.
This is terrifying, and broken:
char * s = malloc(snprintf(NULL, 0, "%s/%s", cwd, argv[1]) + 1);
sprintf(s, "%s/%s", cwd, argv[1]);
Passing NULL to snprintf, first of all, should be a huge red flag for you. If it's not, train yourself to recognize it. I realize what you're trying to do, but it relies on non-portable behavior. In particular, from the manual:
The glibc implementation of the functions snprintf() and vsnprintf() conforms
to the C99 standard, that is, behaves as described above, since glibc version
2.1. Until glibc 2.0.6 they would return -1 when the output was truncated.
So until glibc 2.0.6, your program would be allocating 0 and then sprintfing into it. Always be careful when you read the manual, to read everything. And even then, program defensively, and train yourself to watch for errors like this one. In this case, passing NULL as the destination argument was an immediate no-no.
In this function, you really should be checking errno (for ENOENT). stat(2) can fail for all sorts of reasons, one of them is that the entry is not there.
bool dirOrFileExists(const char dir[]) {
struct stat st;
if (stat(dir, &st) == 0) {
return true;
}
else {
return false;
}
}
Same for this loop, check errno to make sure the function's failing for the reason you think it should.
while((entry = readdir(mydir)))
Here, you have something a little messy:
int fileOrDirectory(const char path[]) {
struct stat s;
if (stat(path, &s) == 0) {
if (s.st_mode & S_IFDIR) {
// Is a directory
return 0;
} else if (s.st_mode & S_IFREG) {
// Is a file
return 1;
} else {
// Something else
return -1;
}
}
else {
// Error
printf("Error occurred\n");
}
}
First of all, returning values only you know about is bound for destruction. You should define an enum if you want a function to return specific states. Better yet, just do these checks in the function that would normally call this. It's not so bad usually, and you're probably calling stat(2) more than you should. It's a syscall, and those can be expensive. But more importantly, you have a branch where all you do is print something (to stdout, not stderr as you should), and then just return nothing! If you hit an error state, you need to either propagate that error up and handle it in the caller, or crash immediately. Don't leave your program in a garbage state and keep going. A compiler should have warned you about this (not having a return value in one branch). Turn on -Werror and at least -Wall. And fix your warnings, don't try to suppress them.
You should use fscanf(3) instead of this:
while (1) {
ch = fgetc(fp);
newch[i] = ch;
if (ch == EOF) {
newch[i] = '\0';
break;
}
i++;
}
That's all for now, I gotta go. Good luck!
- fsaintjacques 14y agoI'm not the author but I would like to thank you for the constructive comment. You make the internet a better place.
- databus 14y agoI see 17 mallocs and not one free!
- JoachimSchipper 14y agoThat's acceptable for a command-line program that will terminate quickly. It's debatable whether this is good style, but it doesn't actually hurt.
- derleth 14y ago> That's acceptable for a command-line program that will terminate quickly No, it isn't; you don't know how quickly the program will terminate, in general, and it only takes one program running longer than you expect to eat a horrible amount of RAM.
- JoachimSchipper 14y agoNot in general, no, but this particular program doesn't allocate a serious amount of memory (think `ls | less`) and doesn't contain any system calls which can reasonably block[1], so it's extremely unlikely to bite you in this case. (I do think it's bad style, and would never write this program this way.) [1] Assuming you stay away from the Zombie^W Network File System. But NFS mounts going away is bad news anyway, this program holding an extra KB of swappable memory doesn't really intensify the pain.
- tptacek 14y agoI'll make a stand for it being bad style to free() memory when in any reasonable run of the program exit() would free just as effectively. This is simply arena allocation; some very excellent, well-regarded C code uses arenas and pools in the same idiom. The downside to calling free(), apart from performance (malloc/free are extremely expensive), is that messing them up creates bugs that are much worse than simply holding on to some allocator metadata until the program hits exit().