hmm. It should be. You may just have to run brew update? It wasn't added very long ago so you may not see it if brew has not been updated. If it still doesn't work then not sure.
Are you sure you committed it to the homebrew repository on github? Your github page doesn't show any activity that indicates you've forked the homebrew repo or created a pull request in the past 14 days (since you made the "c" repo).
Even so, based on the general rules for homebrew inclusion they would reject the formula because:
1. I don't think they like names that have the potential to conflict with others ("c" is in that category). The shortest name I see in their repo is two chars.
2. Typically they only want widely used formulas ("We generally frown on authors submitting their own work unless it is suitably notable." [1]).
hmm interesting. I just followed the instructions on http://mxcl.github.com/homebrew/ and when I run brew install c in my terminal it says "Error: c-1.0.0 already installed". But if it is not a formula they would want then I guess I should not be too worried.
That just means you have a local formula that knows how to build and install c, but you have not shared it with the community so no one else will have access to the formula. You'd need to fork, push to your fork, and create a pull request to the main homebrew repo to try for inclusion.
Also, it is only my hunch that they wouldn't accept the formula--you can still try. I'd still rename the project to be more descriptive (dir-c?) as there are only two homebrew formulae that have single char names (R, which has been around since 1993 and Z, which I'm surprised got included).
ha, no, afraid it didn't. I have installed other programs from brew that were one character. But I plan to rename it just to comment and then provide instructions on how to alias it to c if one would wish. Thanks for warning.
And yeah, didn't think of that. It would be neat to be able to tag files or directories and then execute a command to see all the directories with that tag and then enter that directory easily. Maybe i'll try adding that to the next version. Can you think of anything else that would be useful?
There is no reason to use clear during a terminal session. ^L (Ctrl-L) does this for you in one key press, without needing to clear your input or fill your history.
> There is no reason to use clear during a terminal session. ^L (Ctrl-L) does this for you in one key press, without needing to clear your input or fill your history.
Isn't this based on the assumption that one's shell explicitly supports ^L or has editline/readline support?
It is. But I plan to start using it. I just started using twitter and only really plan to use it to share projects I work on or communicate with others!
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.
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.
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.
> 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.
Not 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.
I'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().
Messing them up exposes bugs that are likely to bite you in other ways. If you can't get malloc()/free() right, you don't really 'get' the flow control/data flow through your program yet and that is a problem.
Arena allocation is inherently simpler and less error-prone than demand-allocating and demand-freeing. It's not valid to say that arena allocation is "hiding" bugs; what it's doing is foreclosing on the possibility of having those bugs.
Virtually no large project has ever gotten malloc/free completely right in its first revision; for the past 20 years or so, most projects get this wrong to the tune of "remote code execution".
The upside to calling free() is that it is a valuable lesson in understanding memory lifetimes for a beginner programmer. You can't know when it's good to omit free until you know what it means.
I agree in theory. In practice, programs evolve and code gets reused. Omitting free calls because you expect the code to exit rapidly is a recipe for disaster once you change your mind later. It's much easier to put this stuff in from the start than it is to retrofit it later.
Some additional stuff which jumped out at me, on top of leif's excellent collection:
+ cwd[] should be MAXPATHLEN bytes long
+ if you want people to use it, you need to put some license on it. GPL and ISC (simplified BSD) are good choices.
+ you may wish to consider asprintf() and err() (both of which elegantly solve a problem that leif has pointed out). These functions are non-POSIX but widely supported and easily portable. (You can also learn how to use the autotools to handle OSes that do not have these functions.)
+ getopt_long() may be easier than parsing parameters "by hand", especially once your utility grows more parameters.
+ hardcoding the colours is ok for now, but you should really use terminfo to find the appropriate escape sequence. The "magic word" for setting the foreground colour is "setaf"; you may wish to look at terminfo(3) and the source code for tput, if your OS has that utility. Warning: proper terminal handling is complex.
This is actually a common mistake — PATH_MAX and the like are not guaranteed to be defined. In fact, in POSIX it is literally impossible to ever know whether your buffer is long enough. Your best bet is,
#ifdef PATH_MAX
char cwd[PATH_MAX];
long len = sizeof cwd;
#else
char *cwd;
long len;
if((len = pathconf(".", _PC_PATH_MAX)) < 0)
len = 8192; /* some arbitrary value */
cwd = malloc(len);
#endif
This is probably one of the ugliest parts of POSIX.
I think it's a little strange to take someone to task for using C99 snprintf and then turn around and tell them to use asprintf. The reality is that both snprintf and asprintf are equally portable --- there was a window in the '90s where snprintf was by itself not portable, and projects routinely carried it along.
This is all academic though; dynamic sizing for a buffer containing a Unix path passed in on the command line? Professional code would just use PATH_MAX or whatever and be done with it. I see downthread that there are concerns about path limits, but since so much of the rest of Unix doesn't care about that particular problem, I'm not sure what the win is trying to optimize it.
> So until glibc 2.0.6, your program would be allocating 0 and then sprintfing into it.
glibc 2.0.6 was released in 1997. I do not think it is reasonable to ask that applications accommodate bugs in libc that have been fixed for 15 years. If someone was actually using a system that old, their system will have so many unpatched vulnerabilities that this will be the least of their problems. And it's highly unlikely that every such bug is documented in the manpages anyway.
Supporting implementations that do not conform to the language standard might be very important to you or it might not. I hardly think it's a moral imperative.
It's not really a "bug". C99 adopted snprintf(), which had become a de facto standard in POSIX code prior to C99, and with it adopted a feature (formatting "into" NULL to get the length of the required buffer) that was not universally implemented.
It's a portability issue.
If you really truly cared about maximum portability, you'd just take a libc snprintf() that did the NULL-size thing and bundle it with your project, just like the AIX version of Sendmail bundled snprintf() because the platform didn't have it.
Or, even better: JETTISON THE PRINTF FAMILY and replace it with something that lets you register your own format codes. This is all ANSI C string manipulation and this code compiles small and in milliseconds.
But in this case the whole discussion is silly because the dynamic string buffer here is not only overengineered (both Unix paths and command line arguments have size restrictions and so dynamic sizing is silly) but also non-idiomatic.
This is awesome, thanks! I just started learning C and am just beginning going through "The C programming language" so thanks for the advice! I definitely plan to improve the program, or feel free to submit a revision!
You should also get in the habit of compiling with warnings (-Wall -Wextra -Werror) and running with valgrind. And writing test programs (in this case they could be shell scripts, unless you want to split your code into a library and an executable, so you can test the library with c programs. That lets you run your tests with valgrind.
4DOS used to have this. It was especially useful back in the day with the 8.3 DOS filenames, which often weren't very descriptive. It's nice to see this feature return.
4DOS used to have a similar feature. It was especially useful back in the day with the 8.3 DOS filenames, which often weren't very descriptive. It's nice to see this feature return.
Hidden files are handy for some things, but a README.txt file would be apparent to anybody who comes across the directory, and not just someone who knows that the hidden directory is/may be there.
I'd prefer to have all the comments located in a single location (e.g. ~/.comments or ~/Dropbox/comments-<hostname>) rather than litter hidden files all over the place because I could (1) easily back them up, (2) share them (even across different platforms), and (3) allow multiple users to set local and global (shared) comments.
As a sub-novice C/++ programmer myself, it's awesome to see a program like this being discussed. The source code is relatively short, it's a simple enough implementation, and all the feedback in the comments here is teaching me things I wouldn't necessarily be exposed to in a book.
Yeah! I am happy with the feedback. I just made this because I thought it would be useful for me and to help learn C since I just got K&R book. Also, I know people are always looking for open source projects to submit to so this would be perfect to try out as there are most likely (as pointed out in the comments) many things that can be improved!
I love this post and the exchange of comments going on here. The OP is just starting to learn C, and to help learn it got started on a project that they found useful, which I think is one of the best ways to learn programming.
The community then responded by posting helpful feedback, much of which demonstrates the idiomatic way of doing things, which is often one of the most difficult things to learn when picking up a new programming language. Other helpful advice is posted on build options and debug tools.
This kind of constructive and useful feedback is great to see.
65 comments
[ 8.5 ms ] story [ 357 ms ] threadEven so, based on the general rules for homebrew inclusion they would reject the formula because:
1. I don't think they like names that have the potential to conflict with others ("c" is in that category). The shortest name I see in their repo is two chars.
2. Typically they only want widely used formulas ("We generally frown on authors submitting their own work unless it is suitably notable." [1]).
[1] https://github.com/mxcl/homebrew/wiki/Acceptable-Formula
Also, it is only my hunch that they wouldn't accept the formula--you can still try. I'd still rename the project to be more descriptive (dir-c?) as there are only two homebrew formulae that have single char names (R, which has been around since 1993 and Z, which I'm surprised got included).
Tagging support would be cool.
And yeah, didn't think of that. It would be neat to be able to tag files or directories and then execute a command to see all the directories with that tag and then enter that directory easily. Maybe i'll try adding that to the next version. Can you think of anything else that would be useful?
Isn't this based on the assumption that one's shell explicitly supports ^L or has editline/readline support?
i.e., "c 128 A" -> AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.
Pentesters! :)
Bonus point for me trying to filter out C++.
This is terrifying, and broken:
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: 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.
Same for this loop, check errno to make sure the function's failing for the reason you think it should. Here, you have something a little messy: 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:
That's all for now, I gotta go. Good luck!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.
(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.
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().
Messing them up exposes bugs that are likely to bite you in other ways. If you can't get malloc()/free() right, you don't really 'get' the flow control/data flow through your program yet and that is a problem.
Virtually no large project has ever gotten malloc/free completely right in its first revision; for the past 20 years or so, most projects get this wrong to the tune of "remote code execution".
My strategy in c is as soon as I write the word "malloc" I figure out where to put the free.
+ cwd[] should be MAXPATHLEN bytes long
+ if you want people to use it, you need to put some license on it. GPL and ISC (simplified BSD) are good choices.
+ you may wish to consider asprintf() and err() (both of which elegantly solve a problem that leif has pointed out). These functions are non-POSIX but widely supported and easily portable. (You can also learn how to use the autotools to handle OSes that do not have these functions.)
+ getopt_long() may be easier than parsing parameters "by hand", especially once your utility grows more parameters.
+ hardcoding the colours is ok for now, but you should really use terminfo to find the appropriate escape sequence. The "magic word" for setting the foreground colour is "setaf"; you may wish to look at terminfo(3) and the source code for tput, if your OS has that utility. Warning: proper terminal handling is complex.
This is actually a common mistake — PATH_MAX and the like are not guaranteed to be defined. In fact, in POSIX it is literally impossible to ever know whether your buffer is long enough. Your best bet is,
This is probably one of the ugliest parts of POSIX.This is all academic though; dynamic sizing for a buffer containing a Unix path passed in on the command line? Professional code would just use PATH_MAX or whatever and be done with it. I see downthread that there are concerns about path limits, but since so much of the rest of Unix doesn't care about that particular problem, I'm not sure what the win is trying to optimize it.
glibc 2.0.6 was released in 1997. I do not think it is reasonable to ask that applications accommodate bugs in libc that have been fixed for 15 years. If someone was actually using a system that old, their system will have so many unpatched vulnerabilities that this will be the least of their problems. And it's highly unlikely that every such bug is documented in the manpages anyway.
It's a portability issue.
If you really truly cared about maximum portability, you'd just take a libc snprintf() that did the NULL-size thing and bundle it with your project, just like the AIX version of Sendmail bundled snprintf() because the platform didn't have it.
Or, even better: JETTISON THE PRINTF FAMILY and replace it with something that lets you register your own format codes. This is all ANSI C string manipulation and this code compiles small and in milliseconds.
But in this case the whole discussion is silly because the dynamic string buffer here is not only overengineered (both Unix paths and command line arguments have size restrictions and so dynamic sizing is silly) but also non-idiomatic.
EDIT
Because I was bored: https://gist.github.com/2697095
The community then responded by posting helpful feedback, much of which demonstrates the idiomatic way of doing things, which is often one of the most difficult things to learn when picking up a new programming language. Other helpful advice is posted on build options and debug tools.
This kind of constructive and useful feedback is great to see.