magnopus-opensource / connected-spaces-platform

"An interoperable communication library for the spatial internet."
https://www.magnopus.com/csp
Apache License 2.0
89 stars 12 forks source link

#524 Check for null when calling functions that return a pointer #525

Closed magnopus-swifty closed 2 weeks ago

magnopus-swifty commented 3 weeks ago

Modify the templates so functions that return values store the result in a temporary local var. This is then checked for null so a C# null can be returned instead of a wrapped null (which would cause a hard crash if dereferenced).

magnopus-swifty commented 3 weeks ago

This PR changes the output for functions that returns pointers (like GetParentEntity()) from:

        public Csp.Multiplayer.SpaceEntity GetParentEntity()
        {
            var _result = new Csp.Multiplayer.SpaceEntity(
                csp_multiplayer_SpaceEntity_GetParentEntityC_SpaceEntityP(_ptr)
            );

            return _result;
        }

to

        public Csp.Multiplayer.SpaceEntity GetParentEntity()
        {
            var _value = csp_multiplayer_SpaceEntity_GetParentEntityC_SpaceEntityP(_ptr);

            if (_value.Equals(NativePointer.Zero))
                return null;

            var _result = new Csp.Multiplayer.SpaceEntity(_value);

            return _result;
        }
MAG-SamBirley commented 3 weeks ago

Firstly, this change is awesome. Secondly, it looks like this change may have brought some of your other recent changes along for the ride (it has changes to the commit message template and chevron module removal). Can we get rid of those for this PR?

magnopus-swifty commented 3 weeks ago

Firstly, this change is awesome. Secondly, it looks like this change may have brought some of your other recent changes along for the ride (it has changes to the commit message template and chevron module removal). Can we get rid of those for this PR?

Those will get diffed out once those changes are merged in. Unfortunately there isn't a nice way to build on top of other in-progress work with Github.

Edit to add: you can see this now the other PRs were committed - this PR automatically simplified.

mag-richardzirbes commented 3 weeks ago

Do we need to put any methods that could return a null value inside a try-catch block now, with the usage of the helper CheckNativePointer(pointer)? (I only sort-of understand the mustache syntax)

magnopus-swifty commented 2 weeks ago

Do we need to put any methods that could return a null value inside a try-catch block now, with the usage of the helper CheckNativePointer(pointer)? (I only sort-of understand the mustache syntax)

No - if null is a valid return (e.g. in the case of GetParentEntity()) then the C# wrapper will also just return null (rather than an invalid object wrapping a null).

If a method has a return type that's a reference, e.g. const SpaceTransform& GetTransform() const then the CheckNativePointer will ensure that a non-null pointer is returned (it shouldn't ever do so) and will always return a valid object.

magnopus-swifty commented 2 weeks ago

Methods returning a reference will check the reference for non-nullness (only in Debug builds):

e.g. SpaceEntity.GetTransform() calling const SpaceTransform& GetTransform() const:

        public Csp.Multiplayer.SpaceTransform GetTransform()
        {
            var _value = csp_multiplayer_SpaceEntity_GetTransformC_SpaceTransformRC(_ptr);

            WrapperHelper.CheckNativePointer(_value);

            var _result = new Csp.Multiplayer.SpaceTransform(_value);

            return _result;
        }

Methods where null is a valid return will also return null on the C# side, e.g. GetParentEntity().