Prevent segfault in PyKDL when asking for nonexisting segment of chain

[DriesDM]

Added methodcode to the Chain class to throw an IndexError when trying to retrieve a segment that doesn't exist.

[meyerj]

LGTM, thanks for your contribution!

[meyerj]

Actually I am not sure if this is valid in SIP. The /Factory/ annotation indicates that the returned pointer is owned by Python, but actually the returned pointer must not be deleted independently. I wonder whether that annotation was actually correct even for the original version of the method.

Why did you change the return type from const Segment& to const Segment*?

Could anybody who is more proficient with SIP could jump in (@smits)?

[DriesDM]

I did not mean to change the return type, that must have slipped in while I was testing, good catch.

[MatthijsBurgh]

This should also be implemented in pybind version

Added changes myself

[meyerj]

I am not so familiar with PyBind11, but this lambda function returns a const Segment & unconditionally, because the input is a const Chain &. Does it make a difference? Is the returned Segment supposed to have reference semantics or does const-correctness matter at all for the Python bindings?

The previous version had a const and a non-const overload, and the SIP version below only has a non-const implementation of getSegment(nr) and the const declaration has been removed.

From this document in the pybind11 repo I understand that the const-ness of returned values is even cast away by pybind11, and that the returned Segment instance is still mutable in Python, with reference semantics? In that case, would

Suggested change

	chain.def("getSegment", [](const Chain &c, unsigned int nr)
	chain.def("getSegment", [](Chain &c, unsigned int nr)

have worked here, too, to emphasize that the returned Segment is mutable and the Chain instance can be modified through this method?

[meyerj]

It should be noted that this change is not consistent with the C++ version of getSegment(), which deliberately does not implement any checks on the index, probably for performance reasons. In C++ it is up to the caller to make sure that the given index is valid.

orocos_kinematics_dynamics/orocos_kdl/src/chain.hpp

Lines 78 to 96 in a2a2dff

	/**
	* Request the nr'd segment of the chain. There is no boundary
	* checking.
	*
	* @param nr the nr of the segment starting from 0
	*
	* @return a constant reference to the nr'd segment
	*/
	const Segment& getSegment(unsigned int nr)const;
	/**
	* Request the nr'd segment of the chain. There is no boundary
	* checking.
	*
	* @param nr the nr of the segment starting from 0
	*
	* @return a reference to the nr'd segment
	*/
	Segment& getSegment(unsigned int nr);

[MatthijsBurgh]

@meyerj Do you think we should implement such checks for all index functions in PyKDL? I will work on a new PR as this one is outdated.