Skip to content

Remove Boost Dependency #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Remove Boost Dependency #31

wants to merge 4 commits into from

Conversation

Woojin-Crive
Copy link
Contributor

Overview

This pull request eliminates the dependency on the Boost library within the dynamixel_hardware_interface package. By replacing Boost functions with standard C++ equivalents, the codebase becomes more streamlined and reduces external dependencies.

Changes Made

  1. Removed Boost Include: The inclusion of the Boost header has been removed from dynamixel_info.hpp:

    -#include <boost/algorithm/string.hpp>
  2. Added Standard Headers: Included <cstdint> and <sstream> headers to support standard C++ functionalities:

    +#include <cstdint>
    +#include <sstream>
  3. Implemented Custom String Split Function: Replaced the Boost string split function with a custom split_string function:

    // Helper function to split string by delimiter
    std::vector<std::string> split_string(const std::string & str, char delimiter)
    {
      std::vector<std::string> tokens;
      std::string token;
      std::istringstream token_stream(str);
      while (std::getline(token_stream, token, delimiter)) {
        if (!token.empty()) {  // Skip empty tokens
          tokens.push_back(token);
        }
      }
      return tokens;
    }
  4. Updated Parsing Logic: Replaced instances of boost::split with the new split_string function:

    -std::vector<std::string> strs;
    -boost::split(strs, line, boost::is_any_of("\t"));
    +std::vector<std::string> strs = split_string(line, '\t');

    Additionally, added checks to ensure the parsed strings are not empty before processing:

    if (!strs.empty()) {
      // Processing logic
    }

Benefits

  • Reduced External Dependencies: By removing Boost, the package now relies solely on standard C++ libraries, simplifying the build process and reducing potential compatibility issues.

  • Enhanced Maintainability: Utilizing standard libraries makes the codebase more accessible to developers who may not be familiar with Boost.

Testing

All functionalities have been tested to ensure that the removal of Boost does not affect the existing behavior of the package. The custom split_string function has been validated to handle string splitting accurately, matching the previous behavior provided by Boost.

Conclusion

This pull request effectively removes the Boost dependency from the dynamixel_hardware_interface package by leveraging standard C++ libraries and custom utility functions. It enhances the package's maintainability and reduces external dependencies without compromising functionality.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @Woojin-Crive, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request removes the dependency on the Boost library in the dynamixel_hardware_interface package. It replaces the Boost string split function with a custom split_string function using standard C++ libraries. The changes involve removing the Boost include, adding standard headers like <cstdint> and <sstream>, implementing the custom string split function, and updating the parsing logic to use the new function. Additionally, checks are added to ensure parsed strings are not empty before processing.

Highlights

  • Dependency Removal: Removes the Boost library dependency to reduce external dependencies and simplify the build process.
  • String Splitting: Replaces boost::split with a custom split_string function using standard C++ streams.
  • Code Modernization: Utilizes standard C++ features to enhance maintainability and accessibility for developers unfamiliar with Boost.

Changelog

  • include/dynamixel_hardware_interface/dynamixel/dynamixel_info.hpp
    • Removed the Boost algorithm string header include on line 29.
    • Added the cstdint header include on line 22.
  • src/dynamixel/dynamixel_info.cpp
    • Added the sstream header include on line 21.
    • Implemented a custom split_string function to replace boost::split from lines 26-38.
    • Replaced boost::split with the custom split_string function on line 98.
    • Added a check to ensure the parsed strings are not empty before processing from lines 100-114.
    • Replaced boost::split with the custom split_string function on line 124.
    • Added a check to ensure the parsed strings are not empty before processing from lines 126-132.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Boost's gone, C++ stands tall,
No external calls at all.
Streams now split,
Code's benefit,
A cleaner interface for all.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@Woojin-Crive Woojin-Crive requested a review from sunghowoo April 8, 2025 23:15
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to remove the Boost dependency from the dynamixel_hardware_interface package, which is a good step towards reducing external dependencies and improving maintainability. The changes involve replacing the Boost string split function with a custom implementation using standard C++ libraries. Overall, the changes seem well-structured and the custom split_string function appears to be a reasonable replacement. However, there are a few areas that could be improved for clarity and robustness.

Summary of Findings

  • String Splitting Logic: The split_string function could be enhanced to handle multiple delimiters or leading/trailing delimiters more gracefully. Consider adding an option to trim whitespace from the tokens.
  • Error Handling: While the code checks for empty strings after splitting, it does not explicitly handle potential errors during the string conversion (stoi, stod). Adding error handling for these conversions would improve the robustness of the code.
  • Duplicated Code: The string splitting logic is duplicated in two different locations in the same file. Consider refactoring to avoid this duplication.

Merge Readiness

The pull request is a good step towards removing the Boost dependency. However, addressing the identified issues related to string splitting logic, error handling, and code duplication would significantly improve the robustness and maintainability of the code. I recommend addressing these issues before merging. I am unable to approve this pull request, and users should have others review and approve this code before merging.

…file processing

- Updated the split_string function to include an optional trim parameter for whitespace removal.
- Improved error handling in ReadDxlModelFile by adding try-catch blocks for invalid arguments and out-of-range errors during data parsing.
- Ensured that empty tokens are skipped after trimming, enhancing robustness in data processing.
@Woojin-Crive
Copy link
Contributor Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to remove the Boost dependency, which is a good step towards reducing external dependencies and improving maintainability. The changes seem well-structured, with a custom string splitting function replacing the Boost equivalent. However, there are a few areas that could be improved for robustness and clarity.

Summary of Findings

  • Error Handling in String Conversion: The code includes try-catch blocks for std::stoi and std::stod, but the error messages are printed to stderr using fprintf. Consider using rclcpp::Logger for consistent logging within the ROS 2 environment. This would provide better integration with ROS 2's logging infrastructure.
  • String Splitting Logic: The split_string function includes an optional trim parameter. It might be beneficial to handle empty tokens more explicitly or provide an option to include them, depending on the expected use cases. Consider adding a comment to explain the rationale behind skipping empty tokens.
  • Code Duplication: The try-catch block is duplicated in two different sections of the code. Consider refactoring this into a separate function to reduce code duplication and improve maintainability.

Merge Readiness

The pull request is a good step towards removing the Boost dependency. However, the error handling and code duplication issues should be addressed before merging. I am unable to directly approve this pull request, and recommend that another reviewer approves this code before merging. I recommend that the identified issues are addressed before merging.

@robotpilot robotpilot self-assigned this Apr 9, 2025
@robotpilot robotpilot added the enhancement New feature or request label Apr 9, 2025
@robotpilot robotpilot requested a review from yun-goon April 9, 2025 07:18
Copy link
Contributor

@yun-goon yun-goon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! This way, the codebase becomes more streamlined and reduces external dependencies!

@robotpilot robotpilot assigned sunghowoo and unassigned robotpilot Apr 9, 2025
@Woojin-Crive
Copy link
Contributor Author

No need to merge this PR to main. I'm Closing this pr.

@Woojin-Crive Woojin-Crive deleted the feature-remove-boost branch April 21, 2025 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants