4 ms·

Are you people serious? His code is absolutely disgusting: uugh = constmap(&mapuser,x,i); if (!uugh) die_user(x,i); ++i; x += i; xlen -= i; i = byt

Are you people serious? His code is absolutely disgusting: uugh = constmap(&mapuser,x,i); if (!uugh) die_user(x,i); ++i; x += i; xlen -= i; i = byte_chr(x,xlen,':'); if (i == xlen) return; https://github.com/amery/qmail/blob/aa6bf9739209ca76f7f3af0feada5552723a622b/qmail-pw2u.c#L205-L207 https://github.com/amery/qmail/blob/aa6bf9739209ca76f7f3af0f... It's mostly bug-free because it scares bugs away. If I was a bug I wouldn't want to live in there.

jevgeni 12y ago
An eloquent, objective, and thoroughly researched opinion.
cremno 12y ago
What are your issues with it? That the variable names are not descriptive enough to be understood when the code is taken out of context? That the if-substatements aren't compound statements? The last line? What would you have done instead (back then)? Every statement on a new line makes it more difficult to understand it since it belongs together, a macro obscures the code and also has to be named, and a function only for this is (was) too expensive speed and memory wise.
- viraptor 12y ago
  Not the gp, but I agree with him. I dislike everything you listed, but think that even with context it's unreadable. Context is hard to find too since this file has 3 lines of comments only. (For an enum equivalent) Also, I don't buy the "function too expensive" idea - that looks like a crazy micro optimization. If it was needed, I'd say it deserves a comment.
  - cremno 12y ago
    Disliking omitting braces or really short names is just a matter of taste (however both is classic C coding style), but you can easily find out what they store without comments or a longer name. I personally would have chosen l instead of x, which is a pointer to a character in the current read _l_ine (see L202). Can you tell me what comments you're expecting? qmail comes with a bunch of man pages (including ones for functions and they're well written). The file is called qmail-pw2u.c and there's qmail-pw2u(8). The function is called dosubuser, so let's look up subuser in it: > Extra addresses. Each line has the form > sub:user:pre: That gives us a pretty good idea about the purpose of the posted code (plus the previous lines). I agree, a function or macro makes sense (since there are many other equivalent constructs in the same file), but that's "just" giving a bunch of statement a name. Does field_next explain what they do? static int field_next(int *i, char **x, unsigned *xlen) { *++i; *x += *i; *xlen -= *i; *i = byte_chr(*x,*xlen,':'); return i == xlen; } / * later */ if (field_next(&i, &x, &xlen)) return; The rest of the function is writing something. Maybe we should find out the purpose of the program at first? The man page tells us this too: >qmail-pw2u reads a V7-format passwd file from standard input and prints a qmail-users-format assignment file.
russtrotter 12y ago
I can't your extent of sarcasm from the comment, but the final sentence made me cackle: "If I was a bug I wouldn't want to live in there"! ha!
AlexandrB 12y ago
It's extremely terse. It's a question of priorities, seems like djb prioritizes correctness over readability - which is fine, but does limit the size of his audience.

jevgeni 12y ago

An eloquent, objective, and thoroughly researched opinion.

cremno 12y ago

What are your issues with it? That the variable names are not descriptive enough to be understood when the code is taken out of context? That the if-substatements aren't compound statements? The last line? What would you have done instead (back then)? Every statement on a new line makes it more difficult to understand it since it belongs together, a macro obscures the code and also has to be named, and a function only for this is (was) too expensive speed and memory wise.

viraptor 12y ago
Not the gp, but I agree with him. I dislike everything you listed, but think that even with context it's unreadable. Context is hard to find too since this file has 3 lines of comments only. (For an enum equivalent) Also, I don't buy the "function too expensive" idea - that looks like a crazy micro optimization. If it was needed, I'd say it deserves a comment.
- cremno 12y ago
  Disliking omitting braces or really short names is just a matter of taste (however both is classic C coding style), but you can easily find out what they store without comments or a longer name. I personally would have chosen l instead of x, which is a pointer to a character in the current read _l_ine (see L202). Can you tell me what comments you're expecting? qmail comes with a bunch of man pages (including ones for functions and they're well written). The file is called qmail-pw2u.c and there's qmail-pw2u(8). The function is called dosubuser, so let's look up subuser in it: > Extra addresses. Each line has the form > sub:user:pre: That gives us a pretty good idea about the purpose of the posted code (plus the previous lines). I agree, a function or macro makes sense (since there are many other equivalent constructs in the same file), but that's "just" giving a bunch of statement a name. Does field_next explain what they do? static int field_next(int *i, char **x, unsigned *xlen) { *++i; *x += *i; *xlen -= *i; *i = byte_chr(*x,*xlen,':'); return i == xlen; } / * later */ if (field_next(&i, &x, &xlen)) return; The rest of the function is writing something. Maybe we should find out the purpose of the program at first? The man page tells us this too: >qmail-pw2u reads a V7-format passwd file from standard input and prints a qmail-users-format assignment file.

russtrotter 12y ago

I can't your extent of sarcasm from the comment, but the final sentence made me cackle: "If I was a bug I wouldn't want to live in there"! ha!

AlexandrB 12y ago

It's extremely terse. It's a question of priorities, seems like djb prioritizes correctness over readability - which is fine, but does limit the size of his audience.