eProsima / Integration-Service

Apache License 2.0
66 stars 28 forks source link

Fix reference to local binding in lambda capture #196

Closed srmainwaring closed 1 year ago

srmainwaring commented 1 year ago

Partially completes #192

clang reports the attempt to capture the variable declared in the structured binding as an error.

See:

MiguelBarro commented 1 year ago

Cannot reproduce it on godbolt. Are you enforcing -std=c++17?

srmainwaring commented 1 year ago

Are you enforcing -std=c++17?

No, not in this case. Here is the build log for a cmake build of integration_service/core with just the experimental::filesystem patch applied:

% cd integration_service/core
% mkdir build && cd build
% cmake ..
-- The CXX compiler identification is AppleClang 14.0.3.14030022
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Boost: /usr/local/lib/cmake/Boost-1.81.0/BoostConfig.cmake (found version "1.81.0") found components: program_options 
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success
-- Configuring done (2.7s)
-- Generating done (0.1s)
-- Build files have been written to: /Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/build
% make
[  9%] Building CXX object CMakeFiles/is-core.dir/src/runtime/FieldToString.cpp.o
[ 18%] Building CXX object CMakeFiles/is-core.dir/src/runtime/MiddlewareInterfaceExtension.cpp.o
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/src/runtime/MiddlewareInterfaceExtension.cpp:68:5: warning: explicitly defaulted move constructor is implicitly deleted [-Wdefaulted-function-deleted]
    Implementation(
    ^
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/src/runtime/MiddlewareInterfaceExtension.cpp:154:19: note: move constructor of 'Implementation' is implicitly deleted because field '_logger' has a deleted move constructor
    utils::Logger _logger;
                  ^
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/include/is/utils/Log.hpp:103:5: note: 'Logger' has been explicitly marked deleted here
    Logger(
    ^
1 warning generated.
[ 27%] Building CXX object CMakeFiles/is-core.dir/src/runtime/Search.cpp.o
[ 36%] Building CXX object CMakeFiles/is-core.dir/src/runtime/StringTemplate.cpp.o
[ 45%] Building CXX object CMakeFiles/is-core.dir/src/systemhandle/RegisterSystem.cpp.o
[ 54%] Building CXX object CMakeFiles/is-core.dir/src/utils/Log.cpp.o
[ 63%] Building CXX object CMakeFiles/is-core.dir/src/Config.cpp.o
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/src/Config.cpp:1253:76: error: 'mw_name' in capture list does not name a variable
                    std::find_if(middlewares.begin(), middlewares.end(), [&mw_name](const Entry& mw)
                                                                           ^
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/src/Config.cpp:1256:32: error: reference to local binding 'mw_name' declared in enclosing function 'eprosima::is::core::internal::Config::load_middlewares'
                        return mw_name != mw.first && std::find(from.begin(), from.end(), mw_name) != from.end();
                               ^
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/src/Config.cpp:1089:23: note: 'mw_name' declared here
    for (const auto& [mw_name, mw_config] : middlewares)
                      ^
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/src/Config.cpp:1256:91: error: reference to local binding 'mw_name' declared in enclosing function 'eprosima::is::core::internal::Config::load_middlewares'
                        return mw_name != mw.first && std::find(from.begin(), from.end(), mw_name) != from.end();
                                                                                          ^
/Users/rhys/Code/ros2/humble/ros2-ardupilot/src/integration_service/core/src/Config.cpp:1089:23: note: 'mw_name' declared here
    for (const auto& [mw_name, mw_config] : middlewares)
                      ^
3 errors generated.
make[2]: *** [CMakeFiles/is-core.dir/src/Config.cpp.o] Error 1
make[1]: *** [CMakeFiles/is-core.dir/all] Error 2
make: *** [all] Error 2
MiguelBarro commented 1 year ago

I'd rather use this strategy for clarity sake:

        {
            // Check if another middleware relies on types builtin or dynamically loaded by the middleware but not
            // explicit in the configuration file
            const auto& ref_mw_name = mw_name;

            if ( middlewares.end() ==
                    std::find_if(middlewares.begin(), middlewares.end(), [&ref_mw_name](const Entry& mw)
                    {
                        auto& from = mw.second.types_from;
                        return ref_mw_name != mw.first && std::find(from.begin(), from.end(), ref_mw_name) != from.end();
                    }))
            {
                logger << utils::Logger::Level::ERROR
                       << "The middleware '" << mw_name
                       << "' has no types associated" << std::endl;
                return false;
            }
        }
srmainwaring commented 1 year ago

I'd rather use this strategy for clarity sake:

Yes, that's clearer. Rebased on main and updated in 4d37ef4

srmainwaring commented 1 year ago

@MiguelBarro btw - thanks for the rapid turn-around on these PRs. We're working to add ROS 2 service support to ArduPilot and this really helps simplify our cross-platform support for SITL.

MiguelBarro commented 1 year ago

Thanks to you we will fix MacOS support 🍎