Skip to content
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

[core] Cover cpplint for all C++ folders #50583

Open
4 of 14 tasks
dentiny opened this issue Feb 14, 2025 · 4 comments
Open
4 of 14 tasks

[core] Cover cpplint for all C++ folders #50583

dentiny opened this issue Feb 14, 2025 · 4 comments
Labels
core Issues that should be addressed in Ray Core enhancement Request for new feature and/or capability good-first-issue Great starter issue for someone just starting to contribute to Ray help-wanted P2 Important issue, but not time-critical

Comments

@dentiny
Copy link
Contributor

dentiny commented Feb 14, 2025

Description

In PR, I introduce cpplint into precommit hook, so we could detect certain invalid usage before it's merged into main branch.

We plan to do it in a gradual style, basically folder by folder, and finally make sure all C++ folders are covered.
Considering cpplint only checks on changed files, you need to manually trigger a check by intentionally change a file, could follow the steps:

  • Checkin the latest main branch, and install precommit hook
  • Manually make some changes for all the files under a certain folder (see script below)
  • Commit your changes, so cpplint is triggered and able to detect invalid usage
  • Fix any issues you've seen, if you think it valid, you could suppress warnings via https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics
  • After all warnings have been addressed or suppressed, add the clean folder into precommit hook

Folders to clean up can be roughly broken down into:

Feel free to reach out to me if you have any questions!

Use case

Example script to change all C++ files under the given folder

import os

def add_newline_to_files(directory):
    # Walk through all files in the directory
    for root, dirs, files in os.walk(directory):
        for file in files:
            file_path = os.path.join(root, file)
            if not file_path.endswith(".h") and not file_path.endswith(".cc"):
                continue

            # Skip directories, only process files
            if os.path.isfile(file_path):
                with open(file_path, 'a') as f:
                    # Add newline if file is not empty and doesn't already end with one
                    f.seek(0, os.SEEK_END)
                    f.write('\n')
                print(f"Added newline to: {file_path}")

# Example usage
directory = "<your-directory>"
add_newline_to_files(directory)
@dentiny dentiny added core Issues that should be addressed in Ray Core enhancement Request for new feature and/or capability good-first-issue Great starter issue for someone just starting to contribute to Ray P2 Important issue, but not time-critical help-wanted labels Feb 14, 2025
@CheyuWu
Copy link
Contributor

CheyuWu commented Feb 14, 2025

Hi @dentiny, I can help with this.

@dentiny
Copy link
Contributor Author

dentiny commented Feb 14, 2025

Hi @dentiny, I can help with this.

hi @CheyuWu thank you so much for the help! Could you please create a subissue which records which part you want to work on, and link to the main thread?

@CheyuWu
Copy link
Contributor

CheyuWu commented Feb 14, 2025

@dentiny OK, No problem. Thx

@CheyuWu
Copy link
Contributor

CheyuWu commented Feb 16, 2025

I used the following command:

cpplint \
  --filter=-whitespace/line_length,\
-build/c++11,\
-build/c++14,\
-build/c++17,\
-readability/braces,\
-whitespace/indent_namespace,\
-runtime/int,\
-runtime/references,\
-build/include_order \
  src/ray/SOME_DIR/*.h \
  src/ray/SOME_DIR/*.cc

to check the cpplint rules. It might be helpful for other contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues that should be addressed in Ray Core enhancement Request for new feature and/or capability good-first-issue Great starter issue for someone just starting to contribute to Ray help-wanted P2 Important issue, but not time-critical
Projects
None yet
Development

No branches or pull requests

2 participants