4 ms·

ncdu is one of my favourite tools, and Zig one of my favourite languages. For like me who are curious: - C version source: https://code.blicky.net/yorhel/ncdu

ncdu is one of my favourite tools, and Zig one of my favourite languages. For like me who are curious: - C version source: https://code.blicky.net/yorhel/ncdu/src/branch/master/ https://code.blicky.net/yorhel/ncdu/src/branch/master/ - Zig version source: https://code.blicky.net/yorhel/ncdu/src/branch/zig https://code.blicky.net/yorhel/ncdu/src/branch/zig The code is nice and very well commented.

wott 5y ago
> - C version source: https://code.blicky.net/yorhel/ncdu/src/branch/master/ https://code.blicky.net/yorhel/ncdu/src/branch/master/ > 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. 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. strftime(mbuf, sizeof(mbuf), "%Y-%m-%d %H:%M:%S %z", localtime(&t)); 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.
- deleted 5y ago
  [deleted]
- saagarjha 5y ago
  > 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”?
  - wott 5y ago
    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.
    - saagarjha 5y ago
      > 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…
- tejasvi88 5y ago
  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.

wott 5y ago

> - C version source: https://code.blicky.net/yorhel/ncdu/src/branch/master/ https://code.blicky.net/yorhel/ncdu/src/branch/master/ > 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. 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. strftime(mbuf, sizeof(mbuf), "%Y-%m-%d %H:%M:%S %z", localtime(&t)); 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.

deleted 5y ago
[deleted]
saagarjha 5y ago
> 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”?
- wott 5y ago
  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.
  - saagarjha 5y ago
    > 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…
tejasvi88 5y ago
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.