[core] Lint cpp files in core_worker

[dayshah]

Why are these changes needed?

Running clang-tidy on files in core-worker, mostly auto-fixes from clang-tidy, some manual fixes. Want it to be part of ci or pre-commit hook once we lint the core src codebase.

Related issue number

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[dentiny]

These changes looks valid and great, do we plan to merge this PR? Let me know if there's anything I could help!

[dayshah]

These changes looks valid and great, do we plan to merge this PR? Let me know if there's anything I could help!

ya just needed it to go through headers separately and do some manual stuff

[dentiny]

If the ownership is not transferred, what about const ActorHandle&?
Reasons:

• Const reference ensures immutability and non-nullability;
• From my limited programming experience, I almost never see people using const std::shared_ptr<>.

[dayshah]

agree I think const ActorHandle& makes sense, and shared ptr copy avoidance makes basically no effect
But it was just a clang-tidy fix where it's copied and not moved so can be ref, and linters don't consider it to be trivially copyable, i assume because copy means you're still accessing ref counter and doing the atomic increment which can be a little bit of a hit on heavily threaded code (which we don't really have), but still just for the linter.

Later I think it could also make things easier to review to cut down on # of shared ptrs if we see paths where there's no real copies being made, cutting down on # of shared ptrs will make a diff

[dentiny]

But it was just a clang-tidy fix where it's copied and not moved so can be ref

I'm sure using ref instead of shared pointer is better, and clang tidy will not complain about it.
My take on clang-tidy is, it's useful to provide good-enough feedbacks, but not perfect; that's why we need NOLINT.

[dayshah]

I'll make a follow-up pr off of this to change all const shared_ptr& that were changed on this to const &, but want to minimize more manual changes on this

[dentiny]

Feel free to leave a TODO if you plan to followup (just in case we forget :) )

[dentiny]

Would be nice to document the linting command (step-by-step) in the PR descriptor, or somewhere more formal for better visibility and educational purpose :)

[dentiny]

I feel like we should have macros to handle syscall errors.

[dayshah]

added todo comment for follow-up, wanna keep changes minimal to clang-tidy stuff here, ruiyang's comment mentioned trying to keep this limited for review purposes

[dentiny]

a nit on comment, I really prefer to have a macro for syscall, since I think we heavily do syscall in our codebase.

[dayshah]

Would be nice to document the linting command (step-by-step) in the PR descriptor, or somewhere more formal for better visibility and educational purpose :)

installed llvm
for file in *.cpp; do /opt/homebrew/opt/llvm@19/bin/clang-tidy --fix-errors --fix-notes $file; done
let it run for a while, while you do other things
look out for specific checks that failed in output that clangd can't see using clang-tidy such as use-after-move

i also tried various --header-filter but didn't get great results, i found putting extra .clang-tidy file with just -* in directories you don't want to lint can be a potential solution to speed it up

And then I go through manually, have clangd setup locally from https://github.com/ray-project/ray/blob/master/BUILD.bazel#L102-L108. And then manually fix things it highlights / related things i see

I'm also trying to add this (https://github.com/lljbash/clangd-tidy) as a pre-commit hook or in ci and will put these directions on fixing there

[dentiny]

Bad naming... Avoid naming ctor argument and data member the same.

[dayshah]

can do in a follow-up want to minimize more changes here

[dentiny]

Thanks for the effort! Just a few suggestions/comments, happy to discuss.

[rynewang]

Whoa this is impressive. I took a glance and generally LGTM, though I did not read every line.

Can you do a clang-tidy-only patch and another manual patch, so that we can merge the tooling one fast, and then take a deep look at the manual ones?

Is the .clang-tidy change affecting CI?

Lint failure in microcheck

[dayshah]

Whoa this is impressive. I took a glance and generally LGTM, though I did not read every line.

Can you do a clang-tidy-only patch and another manual patch, so that we can merge the tooling one fast, and then take a deep look at the manual ones?

Is the .clang-tidy change affecting CI?

Lint failure in microcheck

1. The tried to keep the manual changes I made here minimal, a lot involved just things clang-tidy may have not fixed "correctly" or things that had to change to make it compile with its auto-fixes. I think the main things I did manually was adding a couple extra moves into function captures and removing const from member variables.
2. .clang-tidy isn't used at all in ci rn, I just missed formatting a file, updated

[rynewang]

merge conflict

[dentiny]

Thanks for the effort!

If you don't mind, when you have some time, could you please have a short doc how to play with clang tidy and do some education? playing clang-tidy with bazel requires some setup, for example, the compilation server.

[rynewang]

merge conflict

[dayshah]

merge conflict

updated