Comment by jsnell | Hacker News Reader

Comment by jsnell | Hacker News Reader

Parent Full thread

How many errors are there in the original "simple" code? I mean dangerous and non-obvious logical ones, not the trivial syntax errors. The ones I can see are:

First, the computation of uvs_size is wrong; it's using sizeof(indices[0]) rather than sizeof(uvs[0]). Since the the former is an int and the latter is a Vector2 (presumably 16 bytes), the system doesn't allocate enough memory and there will be a heap overflow.

Second, it tries to do pointer arithmetic on void pointers which is undefined.

Third, it doesn't take into account alignment requirements. For example the Vector2 class would ideally be defined as 16-byte aligned so that the Vector2's can be operated on by SSE instructions. This code gets that wrong or right depending on the number of integers in the adjacent array. So in the worst case it'll be lovely code that works until a data file is changed, and then suddenly starts crashing in odd places.

Sorry, screw raw pointer arithmetic when 7 lines of code have three non-obvious errors, one of which is extremely serious. It seems like a pretty good counterargument to your claim that using raw pointers is not error prone.

Guys, there are errors in this code because I just typed it into a text buffer without ever compiling or running it. It is an example from a slide that I didn't care very much about, for illustration purposes only.

If you did that with the C++ code in this article there would be errors too. Major straw man.

Wasn't your whole point with this language feature that the pointer manipulation code is really awkward to write, and so hard to read that you're never quite sure whether it works or not? :-)

The bug where you copy a line, need to change a bunch of spots and miss one is a common pattern. It happens even when writing real code into a real editor/IDE, and especially when dealing with code that has lots of repetition (as advocated by Galanwe). It's also usually not detected by the compiler, and detected at runtime by simple smoke tests only if you're lucky.

So I don't think it's a strawman argument. It's exactly what you'd expect to happen at some point when using a style that encourages copy-paste programming. You can easily find lots of examples of similar bugs in the wild, in production code that has been running for months or years. It's just fortunate that the example code had an instance of that bug pattern.

Yes, indeed that was a big part of my point!

But also, even if I didn't have a choice and had to write the code in C++, I would do it in the style on the slide, not the style endorsed by the link on this HN thread.

I do agree that copying a line and changing spots is a common mistake pattern. However, I also still think that in many cases that is the best thing to do because it results in the simplest code. So rather than go through contortions in the actual code to try and prevent this, I am wondering if some kind of IDE pattern matching is a better way to catch this class of errors.

FWIW, I assumed as much when I wrote the post. I wrote merely that the code is bug-prone generally, without pointing out specific bugs. A stance that I got the impression you agreed with, from watching your video.

In generally I tried to avoid an overly negative tone when referencing your code or statements, I hope you find that it's reflected in the post.

Yeah, my comment was not in response to your post, it was to the comment I was replying to.

The number of errors there is frightening. I only noticed the use of void-pointer arithmetic and lack of alignment processing.

I would say the code doesn't go far enough. Instead of using `std::unique_ptr<char[]>`, it should define some kind of memory_block abstraction (using `unique_ptr` internally), where the copy constructor and `operator=` both do a byte-wise copy of the data. Then alongside `make_contiguous`, you would have an analogous `copy_contiguous`, where the base case of `make_contiguous_helper` would instead call `memory_block.resize(total_size)` and the base case of `copy_contiguous_helper` would call memory_block = other_memory_block. This would remove the boiler-plate code for the copy constructor in Mesh, which can instead be:

    Mesh::Mesh(const Mesh& other)
      : memory_block(copy_contiguous(other.memory_block,
                                     positions, other.positions,
                                     indices, other.indices,
                                     vertices, other.vertices))
    {
    }

and similar for `operator=`.

I considered that. The problem is that memory_block would need an extra pointer to know its size, which seemed like pure waste at that point. And ultimately it doesn't relieve you of writing copy constructors etc which should be the real goal; it scales much better if you have a class with other members that are not a part of all this contiguous stuff. I hope to solve that in the next post. I think your approach is good as well, just giving you my reasoning for not putting it in.

Rather than two pointers I would probably have pointer + len. Storing the length of your arbitrary memory block is no bad thing, imo.

In any case, having a sensible copy constructor on the memory_block (and any necessary associated data) would at least make the copy constructors in the client classes (assuming you have more than one user of this pattern) cleaner.

It's a really good pattern, though. Excellent blog post.

Thanks, I appreciate that. I welcome your feedback on the follow-up as well.

Reply on news.ycombinator.com