I opened the first file in src/: browser.c. Let's take the function browse_draw_mtime() and start picking nits :-)
It has a buffer of 26 chars, which will (for a regular C string) mean 25 characters + 1 NUL terminator.
char mbuf[26];
But in the end, it prints with the printf()-like function:
printw("%26s", mbuf);
the count there is supposed to exclude the NUL terminator. So it should be 25 and not 26. Note that it cannot cause a problem, but it may indicate that the author didn't carefully grasp the exact definition of the format.
Before that, in a branch the mbuf buffer is filled with the strftime() function. This function is stupidly defined by the C standard and POSIX didn't make it better. It is not the author's fault, but one has to account for its flaws.
One would assume that the result string is truncated if it happens that the result would be too long, so that code would be fine. Well, not quite, it just protects from writing after the buffer. The standards say that in cases when the buffer is not long enough, the content of the result string is indeterminate :-/ Actually, it could even be not a string (not a properly terminated string).
So one should check the value returned by strftime(), check if it is 0, and act accordingly.
Again, it is not dangerous, since the printw("%26s", mbuf) won't read after the buffer. But it may write garbage, for example after year 9999, when the expected result string is too long for the buffer.
-------------
Then in the last file, util.c, there are those macros:
#define oom_msg "\nOut of memory, press enter to try again or Ctrl-C to give up.\n"
and
#define wrap_oom(f) \
which at some point does:
write(2, oom_msg, sizeof(oom_msg));
This is going to emit a NUL character after the newline, because sizeof() of a string literal accounts for it. So, to stop after the newline is emitted, it should be sizeof(oom_msg)-1.
> Let's take the function browse_draw_mtime() and start picking nits :-)
Let’s…not? I don’t see why you would read “this code is nice and well commented” and immediately comb the code to try to find dubious “bugs” with it? It might have been mildly relevant if you responded with “no, I don’t think the code is actually that readable, for example look at this part” but to jump in when nobody made any assertions of correctness or safety and then bring up minor problems is just strange. If someone says “I think she is very pretty” do you respond with “let’s look at her parallel parking skills, shall we? Eh, I’ve seen better”?
When I see “this code is nice and well commented”, I open the first file that comes up to see how good it is, how an example it can be. Then what I see there is functions without any comment, with single-letter variables, with little error checking, with hardcoded yet repeated literal values, and with a few mistakes (with or without consequences but that requires some investigation to be assessed).
Then I open a second file and I find a mistake there too. That's in less than 3 minutes, in a review that's just a quickly oversight picking and peaking random excerpts.
So should I have said, "baaaah, this sucks!"? As it nevertheless far from being the worst code ever, I'd rather warn that what I am going to do amounts to nit-picking, and put a smiley to show that I don't want to disparage the work presented to me.
> Then what I see there is functions without any comment, with single-letter variables, with little error checking, with hardcoded yet repeated literal values
Well, you didn't really talk about those things the first time, you tried to find bugs instead…
In the end, downvotes indicate how many people had a strong negative reaction towards a post. It leaves out majority of the aspects worth considering. I found your post insightful so thank you. I wonder if the downvoters press dislike emoji more often during code review.
Well thanks :-) (as I discover it now, there seems to be only a balance of -1, I guess it was more severe when you saw it). What's funny is that the day before, I got like 10 upvotes for saying how, when I occasionally, quickly browsed a few short patches for Linux kernel drivers, I found rather obvious mistakes which should have been caught by reviewers, if they did there work properly. Oh well... :-)
To sum up, I'd say I quickly looked at 2 of the most common sources of mistakes:
- off-by-one errors: here, in the sub-species which concern how the NUL terminator is counted (or not counted), whether it is by the code itself, or by calling standard functions which behave differently in that respect (sometimes for good reasons, sometimes not, but in all cases one should be careful and check their spec twice);
- error management: what happens, what could we get as a result when an input is not in a typical range, how can we deal with it; standard functions are not especially regular in that respect, one should check that aspect of their spec too, for it can be unintuitive (as in the case of strftime() here).
Then I explained how the consequences were not serious in that case. In a well commented code, I'd expect a small comment to explain why this check is not done, why one can emit that without much care, and so on.
Otherwise, one cannot know if it works by design or by chance; also if future modifications should happen with those pieces of code, problems could arise.
Emitting a NUL character to a (pseudo) terminal is not serious. In the worst case it could mess the display for 1 or 2 lines before it recovers, but generally it won't even have a visible effect, the terminal will simply ignore it.
However, if that gets written or redirected to a file (here, a log file for example), then the NUL character is quite present. Since a civilised text file oughtn't to contain a NUL character, a program which expects well-formed text files may not be ready to handle it properly. For example, reading such a file with the standard C function fgets() or similar will land you in a world of troubles. fgets() only stops when it encounters a newline character, so the NUL character will be included inside the string returned by fgets() (here, coming right after a newline, it would be at the start of the next line); but when you'll ask for the string length, it will be 0 (NUL being the first character), and printing the string will be the same as printing an empty string. Basically, you have lost the line content.
So it is better to be as correct as possible from the start. It is also often easier than thinking exhaustively of all possible implications.
> The code is nice and very well commented.
I opened the first file in src/: browser.c. Let's take the function browse_draw_mtime() and start picking nits :-)
It has a buffer of 26 chars, which will (for a regular C string) mean 25 characters + 1 NUL terminator.
But in the end, it prints with the printf()-like function:printw("%26s", mbuf);
the count there is supposed to exclude the NUL terminator. So it should be 25 and not 26. Note that it cannot cause a problem, but it may indicate that the author didn't carefully grasp the exact definition of the format.
Before that, in a branch the mbuf buffer is filled with the strftime() function. This function is stupidly defined by the C standard and POSIX didn't make it better. It is not the author's fault, but one has to account for its flaws.
One would assume that the result string is truncated if it happens that the result would be too long, so that code would be fine. Well, not quite, it just protects from writing after the buffer. The standards say that in cases when the buffer is not long enough, the content of the result string is indeterminate :-/ Actually, it could even be not a string (not a properly terminated string).So one should check the value returned by strftime(), check if it is 0, and act accordingly.
Again, it is not dangerous, since the printw("%26s", mbuf) won't read after the buffer. But it may write garbage, for example after year 9999, when the expected result string is too long for the buffer.
-------------
Then in the last file, util.c, there are those macros:
and which at some point does: This is going to emit a NUL character after the newline, because sizeof() of a string literal accounts for it. So, to stop after the newline is emitted, it should be sizeof(oom_msg)-1.