Add equal / operator == method

[joshuawarner32]

This is a "simplest thing that could possibly work" implementation. I'm fully expecting to reorganize the code, change the types around, etc - after hearing feedback. I'll also clean up the git history at that point, which has rather unhelpful commit messages right now.

[kentonv]

I hate to say this, but changing AnyStruct::getDataSection() to return a uint64_t array rather than a byte array violates C++ aliasing rules and so won't work. The only pointer type that is allowed to alias other data is char (including unsigned char). You cannot actually read or write the contents of a struct's data section as a uint64_t array. For example, say you have:

struct MyType {
  foo @0 :UInt32;
}

And then you wrote:

MyType::Builder b = ...;
b.setFoo(123);
AnyStruct::Builder(b).getDataSection()[0] = 456;
uint32_t foo = b.getFoo();

Will the value of foo end up being 123 or 456? The answer is: it could be either! Since the value 456 is being set through a uint64_t pointer, and getFoo() reads from a uint32_t pointer, the compiler is allowed to assume that the two cannot point to the same value, and therefore the compiler may reorder the assignment to occur after getFoo() (or before setFoo()), and foo might therefore end up being 123.

This is why the capnp::word type is opaque (rather than being a typedef of uint64_t). We use it to clarify pointer arithmetic and alignment, but we never actually read or write a capnp::word value directly; we instead cast to something else, or use memcpy() which effectively operates on bytes.

Another problem with representing the data section as a uint64_t array is that it might lead to endianness-dependent application code.

[joshuawarner32]

I actually wasn't considering saving people from endianness-dependent code, I totally forgot about aliasing rules. Both very good points.

I changed that primarily for chopping off zeroed words at the end of structs (for canonicalization). There's no point in chopping off a byte at a time - it'll have no functional difference, but probably be a little slower. In my hashing implementation, I have very similar code, except it uses a nasty series of casts to get there.

Note that, aliasing rules aside, what I'm doing with the result should be safe (comparing it with 0) - since that's a completely endianness-independent operation. Perhaps capnp::word can allow for that?

Is there room for some sort of middle-ground here, where we can do certain safe operations on them as complete words, but not break aliasing rules?

[joshuawarner32]

I went ahead and reverted back to the original Data::* return types for getDataSection and cleaned up the git history. Let me know if you notice anything else.

[kentonv]

The previous code for isNull() also returned true if pointer itself was null, but you've removed that check here. Better add it back.

[joshuawarner32]

Whoops, not sure how I missed that.

[kentonv]

Thanks for working on this! I've added a bunch of comments. Sorry for the delay -- it's crunch time at Sandstorm.

[joshuawarner32]

Not a problem. I'm rooting for Sandstorm.

I think I took care of everything. I also renamed StructEqualityResult to Equality, since the long name was getting on my nerves. I'm not attached to it if you have a preference.

I squashed the history, since it didn't seem particularly important. I still have the original commits, if you'd prefer (they're actually meaningfully separated this time).

[kentonv]

Hi Joshua,

It's pretty difficult for me to iterate on a large code review like this if I can't see the incremental changes. Would you mind un-smashing them? Sorry I didn't see this earlier.

[kentonv]

(That is to say, the incremental commits from the last time I reviewed the code -- I don't want to re-read the stuff I've seen already.)

[joshuawarner32]

@kentonv no problem. I'll keep that in mind in the future. Pushed.

[kentonv]

Beautiful, much easier to follow, especially with a commit for every comment like that.

Just the one comment left about placement of KJ_UNREACHABLE.

[kentonv]

Thanks!