12 ms·

To those learning C programming "by reading other's code", following is an example of what not to do (either the subtracted or the added line). [Now I wait for

To those learning C programming "by reading other's code", following is an example of what not to do (either the subtracted or the added line). [Now I wait for a deluge or replies about how it's "idiomatic" C. All I have to say to that is, s/ma// ] --- libc-a/memset.c +++ libc-b/memset.c @@ -1,6 +1,6 @@ void *memset(void *_p, unsigned v, unsigned count) { unsigned char *p = _p; - while(count-- > 0) *p++ = 0; + while(count-- > 0) *p++ = v; return _p; } [Edited later to use the code mode]

EdwardDiego 13y ago
What is particularly bad? How would you have done it differently? I've not done much C, so I can't see the issue, although is it perhaps the types? Would count be modified by side-effect in the calling context also?
- chetanahuja 13y ago
  "Would count be modified by side-effect in the calling context also?" Questions like this and others (eg., does the pointer deref operation applies to incremented or non-incremented value of p?) that you have to answer before you can make sense of the code. The cognitive overload on the reader's brain is at least part of the reason that a bug like that went past code-review in memset function on a major platform. MEMSET !! Was the needless cleverness and terseness of the code worth it? P.S. The answer to your question is No. The variable count as used in the body of this function is local to this function (it's passed by value... so it's a copy of what the caller passed).
  - EdwardDiego 13y ago
    Cheers mate, ah, so unless the function declares that it takes a pointer then it's pass-by-val?
    - ggreer 13y ago
      Everything is pass by value in C, but pointers are values. Functions can't modify the value of a pointer, but they can modify whatever's in the memory pointed to by the pointer. To change the pointer itself, you'd have to pass a pointer to a pointer. Functions like asprintf do this. C++ added pass by reference (&), which can be a handy bit of syntactic sugar.
    - chetanahuja 13y ago
      "so unless the function declares that it takes a pointer" What you're saying is correct in the practical sense. But ggreer's answer is the real technically detailed explanation (everything is passed by value, including pointers to memory. But then you can manipulate that memory you have the pointer to. And that makes it effectively pass-by-reference). Also, C++ adds a language defined pass-by-reference operation... so there's that.
  - Dylan16807 13y ago
    Did you just argue that all parameters that will be modified should be copied into new variables? You have an interesting take on C. And I don't see what's 'clever' about decrementing a count instead of incrementing a count. It would be just as easy to assign 0 in a larger function, and I see no way of reducing the cognitive load. This line isn't doing anything complex or weird.
    - chetanahuja 13y ago
      Did you just argue that all parameters that will be modified should be copied into new variables? Can't tell if this question is referring to something I said? If so, I'd like to know what I said that implied such a thing.
      - Dylan16807 13y ago
        You seemed to be labeling the uncertainty of whether modifying count affects the caller 'needless cognitive overhead'. The only way to avoid altering the count parameter is to make a new variable for counting.
        chetanahuja 13y ago
        Well I re-read that comment and it does sound like I'm lumping the call-by-value question with the other needlessly complex stuff (Mainly single line while loop depending on multiple operator precedence issues). Well let me correct myself, I didn't mean to do that. The single line while loop there is just bristling with pitfalls. I'd reject such code in code review and ask it to be rewritten in more readable C. As an added bonus, this exercise would probably also have caught the original bug that started this discussion. The pass-by-value question came from someone who hadn't written C code in years and I'm pretty sure a practicing C coder (in production code) would not have that question in the first place. Or if it does come from a fresh programmer just getting started, it'll only take one code review to get that straight (and probably earn that programme's work some added scrutiny for some time).
- deleted 13y ago
  [deleted]
- bsder 13y ago
  Everything. The information density of the statement is enormous. Do you know the precendence of --, > , ++ and = by heart? Also if you lose a space you are relying on compiler maximal munch to process (count-->0) as --, > and not -, ->. Did p get incremented or did p get incremented? etc. Finally, the same people who write that kind of code also write things like f(x++, (x+1)) without realizing that this is undefined in C. Also, as an added bonus, postincrement/decrement sometimes creates extra unwanted copies in C++ so you need to use the pre- versions. Compilers are good enough to handle tight loops like this even on very low optimization settings. Better to write that stuff out. I have found that as I program more, I kind of agree with Crockford that ++, and -- are evil and should always be replaced by pp += 1; What that does is force you to avoid actions in conditional expressions and pull the mutators inside the block. It also forces you to be explicit about when you expect the increments to happen.
  - userbinator 13y ago
    > Do you know the precendence of --, > , ++ and = by heart? > Also if you lose a space you are relying on compiler maximal munch to process (count-->0) as --, > and not -, ->. Those are fundamental things that should be second-nature to you if you claim to really know C, just as a mathematician would know that exponentiation has higher precedence than multiplication which has higher precedence than addition. > Did p get incremented or did p get incremented? There's an easy and concise mnemonic for the precedence of ++/-- and dereference - just remember that this is a strcpy, as shown in K&R: while(*dst++ = *src++) ; So here it is the pointer that is incremented, and not what it points to. > Finally, the same people who write that kind of code also write things like f(x++, (x+1)) without realizing that this is undefined in C. Those are the people who think they know C, but actually don't; it's an issue of not knowing what you don't know, and in such situations you should either seek to find the answer, or be conservative.
    - chetanahuja 13y ago
      "Those are fundamental things that should be second-nature to you if you claim to really know C" That's a bogus argument. There's an easy way to write code that's readable and maintainable by mixed teams and then there's this macho way of showing off to no particular advantage. - just remember that this is a strcpy, as shown in K&R: while(dst++ = src++) ; Or -- just don't. Just write code that doesn't rely on every reader to mentally clear through a dense thicket of precedence and side-effects to understand your strcpy and get your job done in a pragmatic fashion with 4 lines of easily understood C code instead of 1 line that makes your heart glow with momentary pride.
jleader 13y ago
I was going to complain about the 4 errors in 4 lines of code, until I realized that HN converts asterisk, text, asterisk into italicized text. I assume you wanted your code to look like this: void *memset(void *_p, unsigned v, unsigned count) { unsigned char *p = _p; - while(count-- > 0) *p++ = 0; + while(count-- > 0) *p++ = v; return _p; } In which case, I'm not sure what you're complaining about. Is it that memset here has a non-standard signature (v is supposed to be int, not unsigned)? Or that the "before" code ignored v? Or the un-braced loop body on the same line? Or is the some other problem I'm overlooking (which is possible, it's been about a decade since C was my main day-to-day language)? [Edited to try to coax better text formatting from HN]
- chetanahuja 13y ago
  Thanks for the formatting. I fixed my comment too. As for the bad legibility of the code as written (not due to formatting), where do I start... Oh I know, the writer obviously wants very very badly to write this while loop as a one-liner. That's the start of all the other badness. As a reader of this code now, I have to keep every possible combination of post-increment side effects and precedence rules in mind before this code makes sense. And yes, I know code like this is quoted in books as example of how cleverly you can write "powerful" one-liners. As far as I'm concerned, it's just a dumb show of alpha-coder pride and it's not a surprise to me that a serious bug was buried here.
jsoffer 13y ago
I think it's ok. I was going to say "He should have used memset instead of this low level memory walking that belongs inside the implementation of... oh I see". This is a critical function; doing it the proper "machine-workings, helping the compiler" way is more important than having a first read immediate understanding. Not that an experienced C programmer won't have it anyway.

EdwardDiego 13y ago

What is particularly bad? How would you have done it differently? I've not done much C, so I can't see the issue, although is it perhaps the types? Would count be modified by side-effect in the calling context also?

chetanahuja 13y ago
"Would count be modified by side-effect in the calling context also?" Questions like this and others (eg., does the pointer deref operation applies to incremented or non-incremented value of p?) that you have to answer before you can make sense of the code. The cognitive overload on the reader's brain is at least part of the reason that a bug like that went past code-review in memset function on a major platform. MEMSET !! Was the needless cleverness and terseness of the code worth it? P.S. The answer to your question is No. The variable count as used in the body of this function is local to this function (it's passed by value... so it's a copy of what the caller passed).
- EdwardDiego 13y ago
  Cheers mate, ah, so unless the function declares that it takes a pointer then it's pass-by-val?
  - ggreer 13y ago
    Everything is pass by value in C, but pointers are values. Functions can't modify the value of a pointer, but they can modify whatever's in the memory pointed to by the pointer. To change the pointer itself, you'd have to pass a pointer to a pointer. Functions like asprintf do this. C++ added pass by reference (&), which can be a handy bit of syntactic sugar.
  - chetanahuja 13y ago
    "so unless the function declares that it takes a pointer" What you're saying is correct in the practical sense. But ggreer's answer is the real technically detailed explanation (everything is passed by value, including pointers to memory. But then you can manipulate that memory you have the pointer to. And that makes it effectively pass-by-reference). Also, C++ adds a language defined pass-by-reference operation... so there's that.
- Dylan16807 13y ago
  Did you just argue that all parameters that will be modified should be copied into new variables? You have an interesting take on C. And I don't see what's 'clever' about decrementing a count instead of incrementing a count. It would be just as easy to assign 0 in a larger function, and I see no way of reducing the cognitive load. This line isn't doing anything complex or weird.
  - chetanahuja 13y ago
    Did you just argue that all parameters that will be modified should be copied into new variables? Can't tell if this question is referring to something I said? If so, I'd like to know what I said that implied such a thing.
    - Dylan16807 13y ago
      You seemed to be labeling the uncertainty of whether modifying count affects the caller 'needless cognitive overhead'. The only way to avoid altering the count parameter is to make a new variable for counting.
      - chetanahuja 13y ago
        Well I re-read that comment and it does sound like I'm lumping the call-by-value question with the other needlessly complex stuff (Mainly single line while loop depending on multiple operator precedence issues). Well let me correct myself, I didn't mean to do that. The single line while loop there is just bristling with pitfalls. I'd reject such code in code review and ask it to be rewritten in more readable C. As an added bonus, this exercise would probably also have caught the original bug that started this discussion. The pass-by-value question came from someone who hadn't written C code in years and I'm pretty sure a practicing C coder (in production code) would not have that question in the first place. Or if it does come from a fresh programmer just getting started, it'll only take one code review to get that straight (and probably earn that programme's work some added scrutiny for some time).
deleted 13y ago
[deleted]
bsder 13y ago
Everything. The information density of the statement is enormous. Do you know the precendence of --, > , ++ and = by heart? Also if you lose a space you are relying on compiler maximal munch to process (count-->0) as --, > and not -, ->. Did p get incremented or did p get incremented? etc. Finally, the same people who write that kind of code also write things like f(x++, (x+1)) without realizing that this is undefined in C. Also, as an added bonus, postincrement/decrement sometimes creates extra unwanted copies in C++ so you need to use the pre- versions. Compilers are good enough to handle tight loops like this even on very low optimization settings. Better to write that stuff out. I have found that as I program more, I kind of agree with Crockford that ++, and -- are evil and should always be replaced by pp += 1; What that does is force you to avoid actions in conditional expressions and pull the mutators inside the block. It also forces you to be explicit about when you expect the increments to happen.
- userbinator 13y ago
  > Do you know the precendence of --, > , ++ and = by heart? > Also if you lose a space you are relying on compiler maximal munch to process (count-->0) as --, > and not -, ->. Those are fundamental things that should be second-nature to you if you claim to really know C, just as a mathematician would know that exponentiation has higher precedence than multiplication which has higher precedence than addition. > Did p get incremented or did p get incremented? There's an easy and concise mnemonic for the precedence of ++/-- and dereference - just remember that this is a strcpy, as shown in K&R: while(*dst++ = *src++) ; So here it is the pointer that is incremented, and not what it points to. > Finally, the same people who write that kind of code also write things like f(x++, (x+1)) without realizing that this is undefined in C. Those are the people who think they know C, but actually don't; it's an issue of not knowing what you don't know, and in such situations you should either seek to find the answer, or be conservative.
  - chetanahuja 13y ago
    "Those are fundamental things that should be second-nature to you if you claim to really know C" That's a bogus argument. There's an easy way to write code that's readable and maintainable by mixed teams and then there's this macho way of showing off to no particular advantage. - just remember that this is a strcpy, as shown in K&R: while(dst++ = src++) ; Or -- just don't. Just write code that doesn't rely on every reader to mentally clear through a dense thicket of precedence and side-effects to understand your strcpy and get your job done in a pragmatic fashion with 4 lines of easily understood C code instead of 1 line that makes your heart glow with momentary pride.

jleader 13y ago

I was going to complain about the 4 errors in 4 lines of code, until I realized that HN converts asterisk, text, asterisk into italicized text. I assume you wanted your code to look like this: void *memset(void *_p, unsigned v, unsigned count) { unsigned char *p = _p; - while(count-- > 0) *p++ = 0; + while(count-- > 0) *p++ = v; return _p; } In which case, I'm not sure what you're complaining about. Is it that memset here has a non-standard signature (v is supposed to be int, not unsigned)? Or that the "before" code ignored v? Or the un-braced loop body on the same line? Or is the some other problem I'm overlooking (which is possible, it's been about a decade since C was my main day-to-day language)? [Edited to try to coax better text formatting from HN]

chetanahuja 13y ago
Thanks for the formatting. I fixed my comment too. As for the bad legibility of the code as written (not due to formatting), where do I start... Oh I know, the writer obviously wants very very badly to write this while loop as a one-liner. That's the start of all the other badness. As a reader of this code now, I have to keep every possible combination of post-increment side effects and precedence rules in mind before this code makes sense. And yes, I know code like this is quoted in books as example of how cleverly you can write "powerful" one-liners. As far as I'm concerned, it's just a dumb show of alpha-coder pride and it's not a surprise to me that a serious bug was buried here.

jsoffer 13y ago

I think it's ok. I was going to say "He should have used memset instead of this low level memory walking that belongs inside the implementation of... oh I see". This is a critical function; doing it the proper "machine-workings, helping the compiler" way is more important than having a first read immediate understanding. Not that an experienced C programmer won't have it anyway.