Advertisement
Falven

Merge sort code review response.

Apr 29th, 2015
341
0
Never
Not a member of Pastebin yet? Sign Up, it unlocks many cool features!
text 2.56 KB | None | 0 0
  1. http://codereview.stackexchange.com/questions/88298/modern-c-compliant-merge-sort/88373?noredirect=1#comment159964_88373
  2.  
  3. Prefer to use move rather than copy semantics.
  4. A+ deifnitely slipped my mind on this, and previous, algorithms.
  5.  
  6. Too many lines of parameters.
  7. Stylistic decision, but my style agrees; the mergesort routine only has 3 parameters, it doesn't even make it to the global print width of 80 characters.
  8.  
  9. Memer types names.
  10. This applies to typedefs within functions too? I have noticed that this is necessary for things at global scope, but I guess it would make it easier on me to just always use it rather than worry about when to use it.
  11.  
  12. Type name.
  13. This is a stylistic conventional decision that I don't think I would abide by.
  14. I don't like mixing pascalcase with lowercase_underscore syntax used by the Standard Library.
  15.  
  16. Don't pass comparator to merge by value.
  17. I ended up "Merging" the merge function ;) with the sort function to optimize it. I haven't used merge anywhere else, plus I would still have std::merge so it's one decision I wouldn't mind doing.
  18.  
  19. Non optimal implementation.
  20. Does using a ternary operator here really help that much more with efficiency? Because you're sacrificing some readibility imo.
  21. If you wanted to inline so much you could also omit the curly braces from the while and make `*out = ...` into `*out++ = ...` :) Although i'm not sure about possible the efficiency improvements that would lead from this.
  22. (I'm still not knowledgeable enough to do the whole "analyze the bytecode" thing that some people do.)
  23.  
  24. Space optimization.
  25. Perform the copies in the merge code. Got it.
  26. Also, I should look into merge sorting linked lists, because I have seen that you wouldn't even require secondary storage for linked lists.
  27.  
  28. Result.
  29. There is only one type of input iterator because we know we are sorting two halves of the same container.
  30. So you're saying we don't even need bidirectional iterators in this case, but rather input iterators?
  31.  
  32. There is no output iterator as the merge is done in place.
  33. Don't you need output iterators simply because, even though the merge is done in place, you need to swap or move around values? Input iterators to my knowledge are for input only.
  34.  
  35. The comparitor does not need a default type.
  36. this is done as part of the sort_merge template.
  37. Ok, good observation, but I merged the functions anyway.
  38.  
  39. I don't pass two ranges.
  40. I use begin/mid/end
  41. You have less overhead by 1 iterator copy, but you lose the ability to merge data from separate arrays?
  42.  
  43. Thanks again for your thoughts!
  44. -Falven
Advertisement
Add Comment
Please, Sign In to add comment
Advertisement