7 ms·

James Hague

35 points by vorador 12y ago

Monkeyget 12y ago
I read the original erroneous code, the mistake and attempted to write a correct solution. I ended up with '(2048 - (size % 2048)) + size', the same expression as the original :/. Us humans just don't do well with edge cases in ranges/bounds. Another seemingly simple task which is easy to trip over is : determining if a time interval overlaps another.
- morsch 12y ago
  His corrected version padded_size := (size + 2047) & ~2047 may be correct or it may not be, I can't really tell quickly because it's so oblique to me. Combining decimal and binary arithmetic like that is a bit too old school for me -- of course I still use it on occasion but I try to avoid it. I guess it's second nature to game devs. What's wrong with padded_size := ceil(size / 2048) * 2048 apart from the fact that it's much much slower but still really really fast in what I can only assume is a part of the source where performance doesn't matter, anyway.
  - innocenat 12y ago
    padded_size := ceil(size / 2048) * 2048 ... may be wrong if size is large that can't be store in mantissa of double/float. Assuming C (or equivalent), I usually do this: padded_size = ((size + 2047) / 2048) * 2048;
    - toolslive 12y ago
      this is generic. You can also take into account that 2048 is a power of 2. So you can shift right and then left again.
      - innocenat 12y ago
        All relevant compilers do that automatically.
  - drv 12y ago
    The corrected version is more recognizable to me as "round"; I believe this is a fairly common pattern in code that needs to round or align to powers of two. However, I'll agree that it is perhaps overly clever. I recently wrote some code that used this pattern, except I had remembered it wrong and wrote something like: padded_size := (size + 2047) & ~2048 (2048 instead of 2047.) Luckily it was caught in code review, but it's worth being a bit more explicit; our solution (aside from fixing the number) was to add ASSERT(padded_size % 2048 == 0) so that it was clear what the code was trying to do.
- mikeryan 12y ago
  I think my default code these for cases where like this where an option is to "do nothing" or "return 0" is to always to have a first line like: if (dont_have_to_do_anything) return;
jgrahamc 12y ago
This was a classic that I ran across: http://blog.jgc.org/2009/01/single-line-assignment-filled-with-epic.html http://blog.jgc.org/2009/01/single-line-assignment-filled-wi...
kirab 12y ago
Code should be written for humans and compiler should optimize the algorithm. If the compiler can't optimize properly then I think one should still write the original algorithm into an adjacent comment. So I would've rather written: int padded_size(int size) int remainder = (size % 2048); bool needsPadding = (remainder > 0); if (!needsPadding) { return size; } return size + (2048 - remainder); }
- scott_s 12y ago
  Bikeshed time! I feel that the bool adds complexity to the logic. I would rather just use an actual comment to indicate what the condition is for: int padded_size(int size) { int remainder = size % 2048; // do we even need padding? if (remainder == 0) { return size; } return size + (2048 - remainder); }
- to3m 12y ago
  Really? This is like using addition in a loop to multiply numbers, I'd say. Nothing wrong with the result, it's just a weird, roundabout way of doing it. Do (size+2047)/2048*2048 if you really can't stand the bitwise operations, though in most cases the fact it's a power of two is important and avoiding bitwise operations obscures that.
  - scott_s 12y ago
    I consider (size+2047)/2048*2048 to be an optimization. If I was to explain the process to another person, I would use the algorithm I wrote above; I would literally say, "If size is already a multiple of 2048, just use that, otherwise, add on enough to make it a multiple of 2048."
nathanb 12y ago
I work on protocol engineering and one of the protocols I support requires fields to be padded at 4-byte word boundaries. The code I inherited uses a grungy macro to solve this problem: #define WORD_ALIGN_LEN(l) (l % 4 ? (((l/4)*4) + 4) : l) So for all you complaining about his solution being difficult to mentally parse: it could be worse. (Our code will divide by 4 and then multiply by 4, truncating the remainder, and then add 4. So if we're sending 6 bytes, we'll divide by 4 to get 1, multiply to get 4, then add 4 to get 8.)

35 points by vorador 12y ago

I read the original erroneous code, the mistake and attempted to write a correct solution. I ended up with '(2048 - (size % 2048)) + size', the same expression as the original :/. Us humans just don't do well with edge cases in ranges/bounds. Another seemingly simple task which is easy to trip over is : determining if a time interval overlaps another.

morsch 12y ago
His corrected version padded_size := (size + 2047) & ~2047 may be correct or it may not be, I can't really tell quickly because it's so oblique to me. Combining decimal and binary arithmetic like that is a bit too old school for me -- of course I still use it on occasion but I try to avoid it. I guess it's second nature to game devs. What's wrong with padded_size := ceil(size / 2048) * 2048 apart from the fact that it's much much slower but still really really fast in what I can only assume is a part of the source where performance doesn't matter, anyway.
- innocenat 12y ago
  padded_size := ceil(size / 2048) * 2048 ... may be wrong if size is large that can't be store in mantissa of double/float. Assuming C (or equivalent), I usually do this: padded_size = ((size + 2047) / 2048) * 2048;
  - toolslive 12y ago
    this is generic. You can also take into account that 2048 is a power of 2. So you can shift right and then left again.
    - innocenat 12y ago
      All relevant compilers do that automatically.
- drv 12y ago
  The corrected version is more recognizable to me as "round"; I believe this is a fairly common pattern in code that needs to round or align to powers of two. However, I'll agree that it is perhaps overly clever. I recently wrote some code that used this pattern, except I had remembered it wrong and wrote something like: padded_size := (size + 2047) & ~2048 (2048 instead of 2047.) Luckily it was caught in code review, but it's worth being a bit more explicit; our solution (aside from fixing the number) was to add ASSERT(padded_size % 2048 == 0) so that it was clear what the code was trying to do.
mikeryan 12y ago
I think my default code these for cases where like this where an option is to "do nothing" or "return 0" is to always to have a first line like: if (dont_have_to_do_anything) return;

jgrahamc 12y ago

This was a classic that I ran across: http://blog.jgc.org/2009/01/single-line-assignment-filled-with-epic.html http://blog.jgc.org/2009/01/single-line-assignment-filled-wi...

kirab 12y ago

Code should be written for humans and compiler should optimize the algorithm. If the compiler can't optimize properly then I think one should still write the original algorithm into an adjacent comment. So I would've rather written: int padded_size(int size) int remainder = (size % 2048); bool needsPadding = (remainder > 0); if (!needsPadding) { return size; } return size + (2048 - remainder); }

scott_s 12y ago
Bikeshed time! I feel that the bool adds complexity to the logic. I would rather just use an actual comment to indicate what the condition is for: int padded_size(int size) { int remainder = size % 2048; // do we even need padding? if (remainder == 0) { return size; } return size + (2048 - remainder); }
to3m 12y ago
Really? This is like using addition in a loop to multiply numbers, I'd say. Nothing wrong with the result, it's just a weird, roundabout way of doing it. Do (size+2047)/2048*2048 if you really can't stand the bitwise operations, though in most cases the fact it's a power of two is important and avoiding bitwise operations obscures that.
- scott_s 12y ago
  I consider (size+2047)/2048*2048 to be an optimization. If I was to explain the process to another person, I would use the algorithm I wrote above; I would literally say, "If size is already a multiple of 2048, just use that, otherwise, add on enough to make it a multiple of 2048."

nathanb 12y ago

I work on protocol engineering and one of the protocols I support requires fields to be padded at 4-byte word boundaries. The code I inherited uses a grungy macro to solve this problem: #define WORD_ALIGN_LEN(l) (l % 4 ? (((l/4)*4) + 4) : l) So for all you complaining about his solution being difficult to mentally parse: it could be worse. (Our code will divide by 4 and then multiply by 4, truncating the remainder, and then add 4. So if we're sending 6 bytes, we'll divide by 4 to get 1, multiply to get 4, then add 4 to get 8.)