Address (some) review comments

CHANGELOG.md

@@ -6,8 +6,8 @@ request related to the change, then we may provide the commit.
 
 This is not a comprehensive list of changes but rather a hand-curated collection of the more notable ones. For a comprehensive history, see the [OpenSim Core GitHub repo](https://github.com/opensim-org/opensim-core).
 
-v4.5.1
-======
+v4.6
+====
 - Added support for list `Socket`s via the macro `OpenSim_DECLARE_LIST_SOCKET`. Accordingly, `Component` and Socket have 
 new `getConnectee` overloads that take an index to a desired object in the list `Socket` (#3652).
 

OpenSim/Common/Component.cpp

@@ -166,32 +166,12 @@ void Component::prependComponentPathToConnecteePath(
     const Component& root = subcomponent.getRoot();
     for (auto& comp : subcomponent.updComponentList()) {
         for (auto& it : comp._socketsTable) {
-            // Check if the root has components for all connectee paths or none
-            // of them. If there is a mix, we have reached an internal error
-            // because the socket connectee names are inconsistent.
-            // TODO is this overkill?
-            // TODO is it even possible to have a mix of connectee paths?
-            std::vector<bool> hasComponentConnectees;
-            hasComponentConnectees.reserve(it.second->getNumConnectees());
-            for (int i = 0; i < it.second->getNumConnectees(); ++i) {
-                hasComponentConnectees.push_back(
-                        root.hasComponent(it.second->getConnecteePath(i)));
-            }
-            const bool allTrue = std::all_of(hasComponentConnectees.begin(),
-                                             hasComponentConnectees.end(),
-                                             [](bool b) { return b; });
-            if (!allTrue) {
-                const bool allFalse = std::all_of(hasComponentConnectees.begin(),
-                                                  hasComponentConnectees.end(),
-                                                  [](bool b) { return !b; });
-                OPENSIM_THROW_IF(!allFalse,
-                                 SocketHasInconsistentConnecteePaths,
-                                 it.second->getName(),
-                                 root.getAbsolutePathString());
-
-                // When the root of the subcomponent does not already have a
-                // component for all connectees of this socket, only then
-                // prepend the subcomponent's path to the connectee paths.
+            // Only apply prepend logic if the socket connection is within the
+            // added subcomponent. To do this, check if the connection is *not*
+            // available in the root component. For list sockets, we assume that
+            // checking the first connectee path is sufficient, since all
+            // connectees in the list must be from the same component.
+            if (!root.hasComponent(it.second->getConnecteePath(0))) {
                 it.second->prependComponentPathToConnecteePath(compPath);
             }
         }

OpenSim/Common/Component.h

@@ -174,21 +174,6 @@ class ComponentAlreadyPartOfOwnershipTree : public Exception {
     }
 };
 
-class SocketHasInconsistentConnecteePaths : public Exception {
-public:
-    SocketHasInconsistentConnecteePaths(const std::string& file,
-                                        size_t line,
-                                        const std::string& func,
-                                        const std::string& socketName,
-                                        const std::string& thisName) :
-        Exception(file, line, func) {
-        std::string msg = "Socket '" + socketName;
-        msg += "' of Component '" + thisName;
-        msg += "' has inconsistent connectee paths.";
-        addMessage(msg);
-    }
-};
-
 class ComponentHasNoSystem : public Exception {
 public:
     ComponentHasNoSystem(const std::string& file,
@@ -924,8 +909,7 @@ OpenSim_DECLARE_ABSTRACT_OBJECT(Component, Object);
     const T& getConnectee(const std::string& name) const {
         // get the Socket and check if it is connected.
         const Socket<T>& socket = getSocket<T>(name);
-        OPENSIM_THROW_IF_FRMOBJ(!socket.isConnected(), Exception,
-                "Socket '" + name + "' not connected.");
+        OPENSIM_ASSERT_FRMOBJ(socket.isConnected());
         return socket.getConnectee();
     }
 
@@ -955,8 +939,7 @@ OpenSim_DECLARE_ABSTRACT_OBJECT(Component, Object);
     const T& getConnectee(const std::string& name, unsigned index) const {
         // get the Socket and check if it is connected.
         const Socket<T>& socket = getSocket<T>(name);
-        OPENSIM_THROW_IF_FRMOBJ(!socket.isConnected(), Exception,
-                "Socket '" + name + "' not connected.");
+        OPENSIM_ASSERT_FRMOBJ(socket.isConnected());
         return socket.getConnectee(index);
     }
 
@@ -986,8 +969,7 @@ OpenSim_DECLARE_ABSTRACT_OBJECT(Component, Object);
     */
     const Object& getConnectee(const std::string& name) const {
         const AbstractSocket& socket = getSocket(name);
-        OPENSIM_THROW_IF_FRMOBJ(!socket.isConnected(), Exception,
-                "Socket '" + name + "' not connected.");
+        OPENSIM_ASSERT_FRMOBJ(socket.isConnected());
         return socket.getConnecteeAsObject();
     }
 
@@ -1017,8 +999,7 @@ OpenSim_DECLARE_ABSTRACT_OBJECT(Component, Object);
     */
     const Object& getConnectee(const std::string& name, unsigned index) const {
         const AbstractSocket& socket = getSocket(name);
-        OPENSIM_THROW_IF_FRMOBJ(!socket.isConnected(), Exception,
-                "Socket '" + name + "' not connected.");
+        OPENSIM_ASSERT_FRMOBJ(socket.isConnected());
         return socket.getConnecteeAsObject(index);
     }
 
@@ -3429,14 +3410,17 @@ const C& Socket<C>::getConnectee() const {
 }
 
 template<class C>
-const C& Socket<C>::getConnectee(unsigned index) const {
+const C& Socket<C>::getConnectee(int index) const {
     if (!isConnected()) {
         std::string msg = "Socket " + getName() + " of type " +
                           C::getClassName() + " in " +
                           getOwner().getAbsolutePathString() + " of type " +
                           getOwner().getConcreteClassName() + " is not connected.";
         OPENSIM_THROW(Exception, msg);
     }
+    using SimTK::isIndexInRange;
+    SimTK_INDEXCHECK(index, getNumConnectees(),
+                     "Socket<T>::getConnectee()");
     return _connectees[index].getRef();
 }
 
@@ -3486,8 +3470,8 @@ void Socket<C>::finalizeConnection(const Component& root) {
 
     } else {
         if (!isListSocket() && getConnecteePath().empty()) return;
-        for (unsigned ix = 0; ix < getNumConnectees(); ++ix) {
-            const auto connecteePath = getConnecteePath(ix);
+        for (int i = 0; i < getNumConnectees(); ++i) {
+            const auto connecteePath = getConnecteePath(i);
             OPENSIM_THROW_IF(connecteePath.empty(), ConnecteeNotSpecified,
                              *this, getOwner());
             ComponentPath path(connecteePath);
@@ -3594,8 +3578,8 @@ void Input<T>::finalizeConnection(const Component& root) {
     } else {
         if (!isListSocket() && getConnecteePath().empty()) return;
         std::string compPathStr, outputName, channelName, alias;
-        for (unsigned ix = 0; ix < getNumConnectees(); ++ix) {
-            parseConnecteePath(getConnecteePath(ix),
+        for (int i = 0; i < getNumConnectees(); ++i) {
+            parseConnecteePath(getConnecteePath(i),
                                compPathStr, outputName, channelName, alias);
             ComponentPath compPath(compPathStr);
             const AbstractOutput* output = nullptr;

OpenSim/Common/ComponentSocket.h

@@ -387,7 +387,7 @@ class Socket : public AbstractSocket {
 
     /** Return a const reference to the object connected to this Socket at the
      provided index. */
-    const T& getConnectee(unsigned index) const;
+    const T& getConnectee(int index) const;
 
     bool canConnectTo(const Object& object) const override {
         return dynamic_cast<const T*>(&object) != nullptr;