WirePointer.Offset setter seems broken.

[KubaO]

WirePointer has this:

public int Offset
{
    get => unchecked((int)_ptrData) >> 2;
    set
    {
        if (value >= (1 << 29) || value < -(1 << 29))
            throw new ArgumentOutOfRangeException(nameof(value));

        _ptrData |= (uint)(value << 2);
    }
}

I'd have thought that the last line of set should be:

_ptrData = _ptrData & ~((ulong)(~3u)) | (uint)(value << 2);

Another one:

public int ListOfStructsElementCount
{
    get => (int)((_ptrData >> 2) & 0x3fffffff);
    set { _ptrData = _ptrData & 0xffffffff00000000ul | checked((uint)value << 2); }
}

The assignment in set should probably be:

_ptrData = _ptrData & 0xffffffff00000003ul | checked((uint)value << 2);

If those are genuine errors, then I should be submitting a PR shortly.

[c80k]

The WirePointer implementation assumes that the operations are called in a particular order. The API tries to clarify this a little bit in that methods which are supposed to be called first are prefixed with "Begin". The mentioned setters are supposed to be called afterwards. The proposed fix would break this order, since it would null bits previously set by BeginStruct / BeginList.

ListOfStructsElementCount works because it refers to the "tag" word (see capnp documentation), which must always be a struct, so the lower 2 bits are always 0.

IMO there is no need to change the implementation. Admittedly, the API documentation could be more precise. There are lots of other traps when working with low-level pointers. E.g. there is nothing that protects you from getting/setting a property which belongs to the wrong pointer kind. Since there is actually no reason to use this struct directly I didn't find it worth the effort to implement heavy-weight protections.

[KubaO]

This was precipitated by our work on porting this code to IEC-61131 ST programming language with Codesys 3 extensions. I have added correct-use checks into our imlementation of WirePointer and it throws when used incorrectly, but I would have gotten tripped by this API use “assumption”. I’ll document it better on my end and port the comments to the C# implementation when I get to it.

As an aside, it also seems to me that there could be some use for an abstract CapnProto implementation test suite where the tests are stored in CapnProto binary structures, easily convertible to json if needed, and then get used to generate code for the target environment and target runtime implementation, but that would be a separate project.

As a further anecdote: The IEC-61131 programming environment essentially has no dynamic memory management – everything needs to be pre-allocated, or on the stack, and all you can do type-safely is to pass things by value or by C++-like references to preallocated values (they do support polymorphism as an extension) so I had to adapt some APIs around the list (de)serializer due to slicing: it’s not quite possible to return polymorphic types, since new instances must be returned by value. In general your code is easy to adapt and very clear, and by the time I’m done I’ll have gone through every line of the C# runtime with a fine-toothed comb, so to speak, so I might have more “nit picky observations” as I do so – partly due to not having a full picture until I examine all of it.

It’s also an “interesting” task to implement it all in a programming environment where the most complex data types supported by the system library are barely better than what you get with C standard library: they barely got native fixed size arrays, and C strings (in a Pascal environment - go figure!), and that’s it. It’s really spartan compared to .Net, but thanks to your judicious use of platform, there isn’t all that much I terms of basic data structures that had to be reimplemented.

Thank you for all the work you are putting into this project. It’s extremely useful!

3 okt. 2019 kl. 12:01 em skrev c80k notifications@github.com:

 The WirePointer implementation assumes that the operations are called in a particular order. The API tries to clarify this a little bit in that methods which are supposed to be called first are prefixed with "Begin". The mentioned setters are supposed to be called afterwards. The proposed fix would break this order, since it would null bits previously set by BeginStruct / BeginList.

ListOfStructsElementCount works because it refers to the "tag" word (see capnp documentation), which must always be a struct, so the lower 2 bits are always 0.

IMO there is no need to change the implementation. Admittedly, the API documentation could be more precise. There are lots of other traps when working with low-level pointers. E.g. there is nothing that protects you from getting/setting a property which belongs to the wrong pointer kind. Since there is actually no reason to use this struct directly I didn't find it worth the effort to implement heavy-weight protections.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.