7 ms·

I hate to nitpick on something so mundane and superficial, but why in the world are people still writing code like this in 2020? while (--len && (dst++ = s

I hate to nitpick on something so mundane and superficial, but why in the world are people still writing code like this in 2020? while (--len && (*dst++ = *src++)) ; Dereferenced post-increments are already confusing enough as-is, and yet here we have 1 pre-increment and 2 dereferenced post-increments happening on top of an assignment in a conditional, all in a single expression. Even as someone who does put an assignment in a conditional once in a while, this still feels 100% unjustifiable to me. It's especially ironic given the premise is that C code has security bugs... if the goal is to avoid that, shouldn't there be even more care taken to avoid this kind of code? EDIT: For those who don't think the readability can be improved... any thoughts on something more like this? Do we really need a compound assignment with three side effects in a conditional modifying the function arguments to make this readable? char *strxcpy(char *restrict dst, const char *restrict src, size_t len) { size_t i; for (i = 0; i < len; ++i) { if (src[i] == '\0') { break; } dst[i] = src[i]; } int input_exhausted = i < len; if (input_exhausted) { ++i; } if (i > 0) { dst[i - 1] = '\0'; } return input_exhausted ? &dst[i] : NULL; }

Tempest1981 5y ago
Homage to K&R? Hard to unlearn that style. How about reading the STL source code? I know it's not deliberately obfuscated, but sometimes feels that way.
- mhh__ 5y ago
  A lot of it is deliberate because they have to avoid name collisions thanks to C++ header files being incredibly stupid
abhishekjha 5y ago
I guess this is so that something like this can be asked in interviews to test how lucky the candidate is on that particular day and hour. Otherwise everybody would pass the interviews.
FabHK 5y ago
I don't know, it could be written more verbosely, but then, this is just so idiomatic. Not sure writing it out makes it much more legible. ETA: yes, your version reads really well. Question though: does it require an extra multiplication and addition for every "[i]" in the code below, or are compilers smart enough to optimise that these days? It seems to me that for sufficiently dumb compilers, your "nice" version would be slower. for (i = 0; i < len; ++i) { if (src[i] == '\0') { break; } dst[i] = src[i]; }
- dataflow 5y ago
  You can just try it and see [1], but tl;dr, no, that problem doesn't occur here. The elements are power-of-2 sized, so at worst there would be a shift instead of a multiplication. On x86 there's 'lea' which could encode the shift-and-add in a single (fast) instruction. (Actually, even 'mov' can encode this. See [1].) And one nice thing about simple linear indexing (with power-of-2-sized elements) is that the compiler could use index registers on x86 (si/di), which can sometimes result in even better code than with pointers. But note that there isn't even a need for a shift to begin with here (let alone multiplication), because it'd be a no-op... since sizeof(char) == 1. So the whole concern is moot. (If these were generic iterators in C++ I'd have coded them differently, both due to the reason you're mentioning and also because I shouldn't assume they'd be randomly indexable to begin with.) [1] https://gcc.godbolt.org/z/dqxj5WzTd https://gcc.godbolt.org/z/dqxj5WzTd
acmj 5y ago
This looks straightforward to me and probably to many C programmers. It is natural to write this as well as other styles you can come up with. Nitpicking is a bigger problem which distracts the attention to the real issue.
saagarjha 5y ago
I can’t answer why other people might do this, but I wrote this code so I can at least give my rationale for it. In short: it leans into C idioms to convey intent. “--len” in a loop condition is a way to say “do this len times”. “*dst++ = *src++” is quite literally the reference strcpy implementation. Taken together, it clearly and quite concisely represents the main operation of this function: do a strcpy, but only up to len characters. There is some extra bookkeeping that needs to be done for errors and null-termination, and that’s delineated from the core part of the code. In all, I read the function as “bail out early if there is nothing to do, otherwise do the copy operation and then fix up the result”. Now, of course you might say that my code is prone to off-by-one errors and the like. And it totally is! This is C and you are going to get string handling wrong, guaranteed. When I was writing this I personally ran it through the typical edge cases and it caught an instance where I (IIRC) was writing an extra NUL when I had exactly filled the buffer previously. These things happen and you need to verify the code yourself to fix it beforehand. But, back to the point, I don’t trust your code any more than I would mine. I mean, you’re also breaking out of a loop on an additional condition and reusing the index variable for later logic. That’s basically a big red flag for “there might be bugs here”. There’s really no way to avoid it. I would argue that the most complex part of this code is actually verifying that the transition between the loop and the error handling that happens afterwards is correct, not the copying bit. Adding a few extra variables splits up the logic a bit but it also gives you more loop invariants to keep track of. Anyways, that’s my 2¢.
- dataflow 5y ago
  I'm not talking about correctness though. I'm talking about readability. There are so many (classical!) principles of readability (and debuggability) that are all being violated in that one function: - Not mixing pre and post increments - Not altering function arguments - Minimizing (not maximizing!!) side effects in conditionals - Putting braces around a loop body - Having fewer entry and exit points (lots of returns especially make debugging hell) - Just minimizing needless mutation in general etc. To give an example, I have such a hard time figuring out what even 'len' is in your code after the loop, and what it implies. I have to think about whether it's signed or unsigned, whether it might wrap around or not (I actually thought you had a bug until I saw the earlier return), whether it ends as zero or something else, and what its relationship to the pointers is. (e.g., whether they all change by exactly the same amount.) Whereas just using a simple index without modifying the arguments just erases all those problems altogether. It's one thing to feel they're both equally likely to be buggy (I actually don't particularly agree on that either, but I see what you're saying and think your viewpoint is fine there), but do you find them equally readable/understandable/debuggable too? Surely you can agree it's easier to read code that modifies just 1 local variable and has 1 return with just 1 extraneous increment than one that modifies 3 arguments simultaneously, misuses the natural point of a loop, and then adds several returns to handle cases with only minor variations? Btw, I'm not even particularly concerned about your ability to write code like this. I'm worried about propagating dangerous practices to other people. IMO, for people (or teams) who can write exceptionally correct code even in an unconventional manner, I just keep my mouth shut and let them do their thing as best as they can... internally. But seeing practices that are known to be error-prone for decades still being exhibited and propagated to us mere mortals who are likely to make predictable (human) errors is what bugs me. Especially when the very goal is to teach people to write safe and secure code!
userbinator 5y ago
but why in the world are people still writing code like this in 2020? To be frank, because some people still value intelligence and understanding. I have been working with C for decades and that code is basically second-nature to me. It reads very naturally. Perhaps it should be you and all the anti-intellectuals who should instead consider why they find it "confusing", and take some personal responsibility to actually try to learn the language instead of complaining that it's "not readable". It's especially ironic given the premise is that C code has security bugs... if the goal is to avoid that, shouldn't there be even more care taken to avoid this kind of code? On the contrary, mental laziness is a bigger source of bugs than anything else. Making code more verbose does not help, and your version has so many more "moving pieces" that I'd say it's harder to understand.
- deleted 5y ago
  [deleted]
blux 5y ago
I disagree, I think that is just the way idiomatic C code looks like and should look like. A post incremented dereferenced pointer for example is an idiom, it is so common that at some point you will immediately recognize it for what it is. These idioms keep the code concise, which has a lot of value in itself, since they make it easier to recognize what the code is all about instead of figuring out code that essentially expresses the same intent but does it in some non-idiomatic way. They are like chords in music, standard positions in chess, etc.
- dataflow 5y ago
  > A post incremented dereferenced pointer for example is an idiom, it is so common that at some point you will immediately recognize it for what it is. That's not my point though. I don't really have much of a problem with *dst++ = *src++; in itself; while it doesn't achieve the platonic ideal of a programming language, I agree it's an idiom that can and should be learned for C. I'm talking about everything beyond that. The fact that that is getting mixed in with so much else in that one-liner. We have a predecrement in conjunction with an assignment in the conditional of an empty loop. Surely that's not a "pattern" at that point? If it were, it would surely be a buggy one without the return preceding it! Do you find it easy to tell what the preconditions and postconditions of that loop are? Like do you find the fact that a length of zero would error obvious? Do you not feel it's a potential trap for people maintaining it, or trying to reuse the code elsewhere? And if I gave you some combination of src/len/dst, would you find it easy to figure out what the variables would be after that loop and verify that they're correct? I definitely don't.
- wruza 5y ago
  I read this “idiom” together with --len as lodsb; stosb; cmp; loopnz. But with time it lost it’s purpose, because if you look at any modern char-star interface implementation, you’ll see pages of multi-arch simd-aware code. And it will never pass a review in a decent team, because neither readability nor performance (the author, with all respect, couldn’t even spell its meaning out correctly N times in a row, which we all couldn’t for some N, give or take one, and weather). This idiom is just a dead relict on a venerable C programmer’s home altar.
liendolucas 5y ago
Thank you for mentioning this! As idiomatic as the original loop might be it still forces someone unfamiliar with C to think about it carefully, on the other side this version is trivial to read and understand. Code should be always designed with readability in mind as we spend much of our time reading and understanding code (if an idiom makes hard to understand something I would probably consider it a bad idiom and avoid the style). EDIT: And no, there is also people that write code like you did regardless the language ;)
jmrm 5y ago
Personally, I try to make my code as easier to read as I can. Not only is faster to read to others, but also faster to analyse when you're trying to fix a bug. At the end of the day, today's compilers are good enough to translate the code to the machine in the most efficient way.

I hate to nitpick on something so mundane and superficial, but why in the world are people still writing code like this in 2020? while (--len && (*dst++ = *s

Tempest1981 5y ago

Homage to K&R? Hard to unlearn that style. How about reading the STL source code? I know it's not deliberately obfuscated, but sometimes feels that way.

mhh__ 5y ago
A lot of it is deliberate because they have to avoid name collisions thanks to C++ header files being incredibly stupid

abhishekjha 5y ago

I guess this is so that something like this can be asked in interviews to test how lucky the candidate is on that particular day and hour. Otherwise everybody would pass the interviews.

FabHK 5y ago

I don't know, it could be written more verbosely, but then, this is just so idiomatic. Not sure writing it out makes it much more legible. ETA: yes, your version reads really well. Question though: does it require an extra multiplication and addition for every "[i]" in the code below, or are compilers smart enough to optimise that these days? It seems to me that for sufficiently dumb compilers, your "nice" version would be slower. for (i = 0; i < len; ++i) { if (src[i] == '\0') { break; } dst[i] = src[i]; }

dataflow 5y ago
You can just try it and see [1], but tl;dr, no, that problem doesn't occur here. The elements are power-of-2 sized, so at worst there would be a shift instead of a multiplication. On x86 there's 'lea' which could encode the shift-and-add in a single (fast) instruction. (Actually, even 'mov' can encode this. See [1].) And one nice thing about simple linear indexing (with power-of-2-sized elements) is that the compiler could use index registers on x86 (si/di), which can sometimes result in even better code than with pointers. But note that there isn't even a need for a shift to begin with here (let alone multiplication), because it'd be a no-op... since sizeof(char) == 1. So the whole concern is moot. (If these were generic iterators in C++ I'd have coded them differently, both due to the reason you're mentioning and also because I shouldn't assume they'd be randomly indexable to begin with.) [1] https://gcc.godbolt.org/z/dqxj5WzTd https://gcc.godbolt.org/z/dqxj5WzTd

acmj 5y ago

This looks straightforward to me and probably to many C programmers. It is natural to write this as well as other styles you can come up with. Nitpicking is a bigger problem which distracts the attention to the real issue.

saagarjha 5y ago

I can’t answer why other people might do this, but I wrote this code so I can at least give my rationale for it. In short: it leans into C idioms to convey intent. “--len” in a loop condition is a way to say “do this len times”. “*dst++ = *src++” is quite literally the reference strcpy implementation. Taken together, it clearly and quite concisely represents the main operation of this function: do a strcpy, but only up to len characters. There is some extra bookkeeping that needs to be done for errors and null-termination, and that’s delineated from the core part of the code. In all, I read the function as “bail out early if there is nothing to do, otherwise do the copy operation and then fix up the result”. Now, of course you might say that my code is prone to off-by-one errors and the like. And it totally is! This is C and you are going to get string handling wrong, guaranteed. When I was writing this I personally ran it through the typical edge cases and it caught an instance where I (IIRC) was writing an extra NUL when I had exactly filled the buffer previously. These things happen and you need to verify the code yourself to fix it beforehand. But, back to the point, I don’t trust your code any more than I would mine. I mean, you’re also breaking out of a loop on an additional condition and reusing the index variable for later logic. That’s basically a big red flag for “there might be bugs here”. There’s really no way to avoid it. I would argue that the most complex part of this code is actually verifying that the transition between the loop and the error handling that happens afterwards is correct, not the copying bit. Adding a few extra variables splits up the logic a bit but it also gives you more loop invariants to keep track of. Anyways, that’s my 2¢.

dataflow 5y ago
I'm not talking about correctness though. I'm talking about readability. There are so many (classical!) principles of readability (and debuggability) that are all being violated in that one function: - Not mixing pre and post increments - Not altering function arguments - Minimizing (not maximizing!!) side effects in conditionals - Putting braces around a loop body - Having fewer entry and exit points (lots of returns especially make debugging hell) - Just minimizing needless mutation in general etc. To give an example, I have such a hard time figuring out what even 'len' is in your code after the loop, and what it implies. I have to think about whether it's signed or unsigned, whether it might wrap around or not (I actually thought you had a bug until I saw the earlier return), whether it ends as zero or something else, and what its relationship to the pointers is. (e.g., whether they all change by exactly the same amount.) Whereas just using a simple index without modifying the arguments just erases all those problems altogether. It's one thing to feel they're both equally likely to be buggy (I actually don't particularly agree on that either, but I see what you're saying and think your viewpoint is fine there), but do you find them equally readable/understandable/debuggable too? Surely you can agree it's easier to read code that modifies just 1 local variable and has 1 return with just 1 extraneous increment than one that modifies 3 arguments simultaneously, misuses the natural point of a loop, and then adds several returns to handle cases with only minor variations? Btw, I'm not even particularly concerned about your ability to write code like this. I'm worried about propagating dangerous practices to other people. IMO, for people (or teams) who can write exceptionally correct code even in an unconventional manner, I just keep my mouth shut and let them do their thing as best as they can... internally. But seeing practices that are known to be error-prone for decades still being exhibited and propagated to us mere mortals who are likely to make predictable (human) errors is what bugs me. Especially when the very goal is to teach people to write safe and secure code!

userbinator 5y ago

but why in the world are people still writing code like this in 2020? To be frank, because some people still value intelligence and understanding. I have been working with C for decades and that code is basically second-nature to me. It reads very naturally. Perhaps it should be you and all the anti-intellectuals who should instead consider why they find it "confusing", and take some personal responsibility to actually try to learn the language instead of complaining that it's "not readable". It's especially ironic given the premise is that C code has security bugs... if the goal is to avoid that, shouldn't there be even more care taken to avoid this kind of code? On the contrary, mental laziness is a bigger source of bugs than anything else. Making code more verbose does not help, and your version has so many more "moving pieces" that I'd say it's harder to understand.

deleted 5y ago
[deleted]

blux 5y ago

I disagree, I think that is just the way idiomatic C code looks like and should look like. A post incremented dereferenced pointer for example is an idiom, it is so common that at some point you will immediately recognize it for what it is. These idioms keep the code concise, which has a lot of value in itself, since they make it easier to recognize what the code is all about instead of figuring out code that essentially expresses the same intent but does it in some non-idiomatic way. They are like chords in music, standard positions in chess, etc.

dataflow 5y ago
> A post incremented dereferenced pointer for example is an idiom, it is so common that at some point you will immediately recognize it for what it is. That's not my point though. I don't really have much of a problem with *dst++ = *src++; in itself; while it doesn't achieve the platonic ideal of a programming language, I agree it's an idiom that can and should be learned for C. I'm talking about everything beyond that. The fact that that is getting mixed in with so much else in that one-liner. We have a predecrement in conjunction with an assignment in the conditional of an empty loop. Surely that's not a "pattern" at that point? If it were, it would surely be a buggy one without the return preceding it! Do you find it easy to tell what the preconditions and postconditions of that loop are? Like do you find the fact that a length of zero would error obvious? Do you not feel it's a potential trap for people maintaining it, or trying to reuse the code elsewhere? And if I gave you some combination of src/len/dst, would you find it easy to figure out what the variables would be after that loop and verify that they're correct? I definitely don't.
wruza 5y ago
I read this “idiom” together with --len as lodsb; stosb; cmp; loopnz. But with time it lost it’s purpose, because if you look at any modern char-star interface implementation, you’ll see pages of multi-arch simd-aware code. And it will never pass a review in a decent team, because neither readability nor performance (the author, with all respect, couldn’t even spell its meaning out correctly N times in a row, which we all couldn’t for some N, give or take one, and weather). This idiom is just a dead relict on a venerable C programmer’s home altar.

liendolucas 5y ago

Thank you for mentioning this! As idiomatic as the original loop might be it still forces someone unfamiliar with C to think about it carefully, on the other side this version is trivial to read and understand. Code should be always designed with readability in mind as we spend much of our time reading and understanding code (if an idiom makes hard to understand something I would probably consider it a bad idiom and avoid the style). EDIT: And no, there is also people that write code like you did regardless the language ;)

jmrm 5y ago

Personally, I try to make my code as easier to read as I can. Not only is faster to read to others, but also faster to analyse when you're trying to fix a bug. At the end of the day, today's compilers are good enough to translate the code to the machine in the most efficient way.