6 ms·

Am I the only one shocked by the lack of comments in the code? It's literally a wall of code.

by ye 13y ago

barrkel 13y ago
How familiar are you with largish C codebases? At this level, you're dealing with an API at the calling end (you should be fairly intimately familiar with the semantics without much reference to the docs if you're working on the implementation) and the functions being calling are almost readable as English. Code should strive to be written such that it doesn't need comments unless it's being clever - and it should avoid being clever if possible. Also, I'm not sure where you saw a "wall of code". I don't see any walls of code, not in the commit diff, nor in the smaller diff in the email. A wall of code, for me, would have to be long (say, 70+ lines - but it depends on the language) and dense (e.g. boolean expressions complex enough to need parentheses to clarify precedence).
- Raphael_Amiard 13y ago
  > How familiar are you with largish C codebases? Well I don't know, let's see some code for postgresql, a quite large C codebase. I took this file at random: https://github.com/postgres/postgres/blob/master/src/pl/plpgsql/src/pl_comp.c https://github.com/postgres/postgres/blob/master/src/pl/plpg... So assuming "what you know" means the same as "what is the best" is probably not a good idea.
- crististm 13y ago
  That's an attitude of "stay the fuck out of my code - you're too stupid to understand it without comments". I've seen it too often in "large code bases" "you should be fairly intimately familiar with the semantics" - do you see a problem bootstrapping that on an obscure code base?
  - __david__ 13y ago
    > …do you see a problem bootstrapping that on an obscure code base? No, I don't. It just requires that you can read C and understand it. Any large codebase (in whatever language) is going to require you to understand certain semantics and idioms, and it doesn't make any sense to document those idioms every single place you use them. Big codebases require context and a comment in some random function isn't going to give you enough context unless it was a tome. Also, did you see the 2 patches linked in the bug report? They significantly changed the code. C is just like that. It's an environment ripe for comment bitrot. And the only thing worse than no comments is incorrect comments. In the end, the code is what matters. If you can read it, you can understand it.
    - crististm 13y ago
      Yes, I've seen, read and kind of felt what the patch does. If you claim you _understand_ the code then I call you on this. The only thing worse about something is the preconceived idea that there is indeed only a _single_ thing worse - all the rest being better or equal. No, worse than missing comments is also _bad code_ which was the case here. And bad attitudes like "my code is prone to bit rot so I won't comment it" or "my code is self-explanatory so I won't comment it" or worse "my code is so special and super optimized and smart that I will not comment it so only super special guys will be able to modify it". And also "I'm a kernel maintainer so I don't have to comment anything"
      - acqq 13y ago
        Excuse me, based on which experience or do you claim that it was bad code? Based on which experience do you claim that there should be more comments? It looks to me that you don't have anything to support what you say, and I'd appreciate you showing me the opposite. Have you actually ever programmed anything for Linux kernel? What have you programmed that would be relevant and give the weight to your (for me dubious) claims?
        asveikau 13y ago
        It was bad code because it dereferenced a user mode pointer without going through copy_from_user/copy_to_user. That much is clear. That's not to say that nobody else has made this mistake or that it won't be made again by otherwise capable programmers. One can talk about this as bad code while still having sympathies for the error. Hate the sin, not the sinner.
        crististm 13y ago
        The issue I raised is not about this particular piece of code or about it being defective - it could have been perfect for what is worth. The deeper issue is about code as it is communicated to the next developer. For various reasons (most - me included) developers do not talk to the next developer but to the compiler. And some other guy comes later who doesn't know about copy_to/from_user shit because he is not exposed to the internals. He might be competent programmer but he lacks exposure. He might also be able to see in a second the dereference but when he is reviewing the code, he keeps a stack of several levels of irrelevant contexts that blurred his view. He could have avoided all that if only the code was being written with him in mind.
- grannyg00se 13y ago
  I'm reading Clean Code (again). I dont see that code as being particularly bad. But it definitely wouldnt follow the axioms in that highly regarded book. I realize it isnt java but can someone familiar with the text comment on the number of parameters in that function and the naming of some of those variables? Relevant quote: "The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justiﬁcation—and then shouldn’t be used anyway."
  - justincormack 13y ago
    Well POSIX requires more than three arguments for some system calls. But around 6 is the limit for most architectures. They are passed in registers for efficiency.
  - unwind 13y ago
    Zero? So not even pure functional programming is OK, since that tends to pass state as function arguments? We should all just happily poke our global state from our niladic functions? Haven't read that book, but color me neon skeptical.
  - mikeash 13y ago
    I find the idea that the ideal function takes no arguments to be astonishing. There are only two kinds of functions that take no arguments: 1. Those that return the same result every time. 2. Those that mutate some internal state (or, roughly equivalently, those that inspect internal state being mutated elsewhere). #1 isn't all that useful in general, although there are obviously cases where it's exactly what you want. For #2, while state is useful at times and a necessary evil in many other cases, it's hardly ideal. Even a single parameter seems decidedly un-ideal. To do useful work in general, you're typically going to want to take two parameters: something to be modified, and the modification to make. This can be done with state (modify in place) or functionally (return a new object with the modifications applied). Once again, having a single argument seems to imply either something not very useful (basically a getter or some similar derive-a-value function) or something relying on mutable state. You need two arguments to achieve the sort of combinatorial power that makes functions interesting and properly useful.
asveikau 13y ago
Wall of code? The entire function shows up in a short diff. It looks like all it does is wrap other functions with nearly identical names. It strikes me as kind of uninteresting glue code written for compatibility with a non-native ABI. The kind that gets repetitive if you're not terse.
nightpool 13y ago
It looks perfectly readable to me, and I'm not even a big C guy. (Though I do have a general familiarity with it, and more with C++). Code shouldn't really need comments in most cases, especially if you're writing for people who are assumed to have a familiarity with the coding style/idioms used (such as boilerplate code for exploiting a memory vulnerability). (Not that I'm saying this is, I don't have enough C experience to say whether it is or not. It just seems reasonable to assume from context)
- crististm 13y ago
  Yeah, you understand the code - we all do. But it doesn't tell you anything about the context the code runs in. And its context doesn't tell you about its context either. Sparse code is not necessarily self-explaining.
  - adestefan 13y ago
    The function names do. Honestly, this is very, very simple code. Most of the code in a kernel is nothing more than book keeping or glue connecting things. This is glue.
    - adestefan 13y ago
      To expand this is also a really dumb mistake. It's the equivalent of passing user input directly into a SQL query.
  - asveikau 13y ago
    There are cues here that are consistent with conventions in the Linux kernel. For example the sys prefix on the function name tells you everything you need to know about "the context the code runs in" - it's a syscall. "compat" gives you a hint that it's a wrapper for another ABI.
    - crististm 13y ago
      "compat" gives you a hint that it's a wrapper for another ABI Why is this obvious? I can extrapolate on sys, but what cues are in 'compat' that I don't see?
crististm 13y ago
I'm no longer shocked. You should see the comments in the kernel crypto API. Oh, there are none...

Am I the only one shocked by the lack of comments in the code? It's literally a wall of code.

by ye 13y ago

Am I the only one shocked by the lack of comments in the code? It's literally a wall of code.

How familiar are you with largish C codebases? At this level, you're dealing with an API at the calling end (you should be fairly intimately familiar with the semantics without much reference to the docs if you're working on the implementation) and the functions being calling are almost readable as English. Code should strive to be written such that it doesn't need comments unless it's being clever - and it should avoid being clever if possible. Also, I'm not sure where you saw a "wall of code". I don't see any walls of code, not in the commit diff, nor in the smaller diff in the email. A wall of code, for me, would have to be long (say, 70+ lines - but it depends on the language) and dense (e.g. boolean expressions complex enough to need parentheses to clarify precedence).

Raphael_Amiard 13y ago
> How familiar are you with largish C codebases? Well I don't know, let's see some code for postgresql, a quite large C codebase. I took this file at random: https://github.com/postgres/postgres/blob/master/src/pl/plpgsql/src/pl_comp.c https://github.com/postgres/postgres/blob/master/src/pl/plpg... So assuming "what you know" means the same as "what is the best" is probably not a good idea.
crististm 13y ago
That's an attitude of "stay the fuck out of my code - you're too stupid to understand it without comments". I've seen it too often in "large code bases" "you should be fairly intimately familiar with the semantics" - do you see a problem bootstrapping that on an obscure code base?
- __david__ 13y ago
  > …do you see a problem bootstrapping that on an obscure code base? No, I don't. It just requires that you can read C and understand it. Any large codebase (in whatever language) is going to require you to understand certain semantics and idioms, and it doesn't make any sense to document those idioms every single place you use them. Big codebases require context and a comment in some random function isn't going to give you enough context unless it was a tome. Also, did you see the 2 patches linked in the bug report? They significantly changed the code. C is just like that. It's an environment ripe for comment bitrot. And the only thing worse than no comments is incorrect comments. In the end, the code is what matters. If you can read it, you can understand it.
  - crististm 13y ago
    Yes, I've seen, read and kind of felt what the patch does. If you claim you _understand_ the code then I call you on this. The only thing worse about something is the preconceived idea that there is indeed only a _single_ thing worse - all the rest being better or equal. No, worse than missing comments is also _bad code_ which was the case here. And bad attitudes like "my code is prone to bit rot so I won't comment it" or "my code is self-explanatory so I won't comment it" or worse "my code is so special and super optimized and smart that I will not comment it so only super special guys will be able to modify it". And also "I'm a kernel maintainer so I don't have to comment anything"
    - acqq 13y ago
      Excuse me, based on which experience or do you claim that it was bad code? Based on which experience do you claim that there should be more comments? It looks to me that you don't have anything to support what you say, and I'd appreciate you showing me the opposite. Have you actually ever programmed anything for Linux kernel? What have you programmed that would be relevant and give the weight to your (for me dubious) claims?
      - asveikau 13y ago
        It was bad code because it dereferenced a user mode pointer without going through copy_from_user/copy_to_user. That much is clear. That's not to say that nobody else has made this mistake or that it won't be made again by otherwise capable programmers. One can talk about this as bad code while still having sympathies for the error. Hate the sin, not the sinner.
        crististm 13y ago
        The issue I raised is not about this particular piece of code or about it being defective - it could have been perfect for what is worth. The deeper issue is about code as it is communicated to the next developer. For various reasons (most - me included) developers do not talk to the next developer but to the compiler. And some other guy comes later who doesn't know about copy_to/from_user shit because he is not exposed to the internals. He might be competent programmer but he lacks exposure. He might also be able to see in a second the dereference but when he is reviewing the code, he keeps a stack of several levels of irrelevant contexts that blurred his view. He could have avoided all that if only the code was being written with him in mind.
grannyg00se 13y ago
I'm reading Clean Code (again). I dont see that code as being particularly bad. But it definitely wouldnt follow the axioms in that highly regarded book. I realize it isnt java but can someone familiar with the text comment on the number of parameters in that function and the naming of some of those variables? Relevant quote: "The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justiﬁcation—and then shouldn’t be used anyway."
- justincormack 13y ago
  Well POSIX requires more than three arguments for some system calls. But around 6 is the limit for most architectures. They are passed in registers for efficiency.
- unwind 13y ago
  Zero? So not even pure functional programming is OK, since that tends to pass state as function arguments? We should all just happily poke our global state from our niladic functions? Haven't read that book, but color me neon skeptical.
- mikeash 13y ago
  I find the idea that the ideal function takes no arguments to be astonishing. There are only two kinds of functions that take no arguments: 1. Those that return the same result every time. 2. Those that mutate some internal state (or, roughly equivalently, those that inspect internal state being mutated elsewhere). #1 isn't all that useful in general, although there are obviously cases where it's exactly what you want. For #2, while state is useful at times and a necessary evil in many other cases, it's hardly ideal. Even a single parameter seems decidedly un-ideal. To do useful work in general, you're typically going to want to take two parameters: something to be modified, and the modification to make. This can be done with state (modify in place) or functionally (return a new object with the modifications applied). Once again, having a single argument seems to imply either something not very useful (basically a getter or some similar derive-a-value function) or something relying on mutable state. You need two arguments to achieve the sort of combinatorial power that makes functions interesting and properly useful.

asveikau 13y ago

Wall of code? The entire function shows up in a short diff. It looks like all it does is wrap other functions with nearly identical names. It strikes me as kind of uninteresting glue code written for compatibility with a non-native ABI. The kind that gets repetitive if you're not terse.

nightpool 13y ago

It looks perfectly readable to me, and I'm not even a big C guy. (Though I do have a general familiarity with it, and more with C++). Code shouldn't really need comments in most cases, especially if you're writing for people who are assumed to have a familiarity with the coding style/idioms used (such as boilerplate code for exploiting a memory vulnerability). (Not that I'm saying this is, I don't have enough C experience to say whether it is or not. It just seems reasonable to assume from context)

crististm 13y ago
Yeah, you understand the code - we all do. But it doesn't tell you anything about the context the code runs in. And its context doesn't tell you about its context either. Sparse code is not necessarily self-explaining.
- adestefan 13y ago
  The function names do. Honestly, this is very, very simple code. Most of the code in a kernel is nothing more than book keeping or glue connecting things. This is glue.
  - adestefan 13y ago
    To expand this is also a really dumb mistake. It's the equivalent of passing user input directly into a SQL query.
- asveikau 13y ago
  There are cues here that are consistent with conventions in the Linux kernel. For example the sys prefix on the function name tells you everything you need to know about "the context the code runs in" - it's a syscall. "compat" gives you a hint that it's a wrapper for another ABI.
  - crististm 13y ago
    "compat" gives you a hint that it's a wrapper for another ABI Why is this obvious? I can extrapolate on sys, but what cues are in 'compat' that I don't see?

crististm 13y ago

I'm no longer shocked. You should see the comments in the kernel crypto API. Oh, there are none...