10000 [1/N] Enable clang-tidy on caffe2/serialize/ by cyyever · Pull Request #141849 · pytorch/pytorch · GitHub
[go: up one dir, main page]
Skip to content

[1/N] Enable clang-tidy on caffe2/serialize/ #141849

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[1/N] Enable clang-tidy on caffe2/serialize/ #141849

wants to merge 1 commit into from

Conversation

cyyever
Copy link
Collaborator
@cyyever cyyever commented Dec 2, 2024

Fixes #ISSUE_NUMBER

@pytorch-bot PyTorch Bot
Copy link
pytorch-bot bot commented Dec 2, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/141849

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 1375b23 with merge base 9df9d9d (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the caffe2 label Dec 2, 2024
@cyyever cyyever marked this pull request as draft December 2, 2024 04:44
@cyyever cyyever force-pushed the seri branch 13 times, most recently from e87e56e to 40fe1bc Compare December 4, 2024 03:17
@cyyever
Copy link
Collaborator Author
cyyever commented Dec 4, 2024

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Dec 4, 2024
@cyyever cyyever force-pushed the seri branch 4 times, most recently from 256b340 to 25b204f Compare December 4, 2024 15:00
@cyyever cyyever changed the title Run clang-tidy and format Enable clang-tidy on caffe2/serial Dec 4, 2024
@cyyever cyyever changed the title Enable clang-tidy on caffe2/serial Enable clang-tidy on caffe2/serialize/ Dec 4, 2024
@cyyever cyyever marked this pull request as ready for review December 4, 2024 15:26
@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Dec 5, 2024
@cyyever cyyever requested a review from Skylion007 December 10, 2024 23:21
@cyyever cyyever requested a review from albanD December 19, 2024 03:05
@albanD albanD removed their request for review December 20, 2024 14:53
@cyyever cyyever requested a review from albanD December 25, 2024 10:36
albanD
albanD reviewed Dec 25, 2024
View reviewed changes
8000
Copy link
Collaborator
@albanD albanD left a comment

Choose a reason for hiding this comment

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

SGTM
Not sure if @mikaylagawarecki has any final comment on this though?

@cyyever
Copy link
Collaborator Author
cyyever commented Feb 2, 2025

@albanD Seems no comment.

albanD
albanD reviewed Feb 3, 2025
View reviewed changes
@cyyever
Copy link
Collaborator Author
cyyever commented Feb 4, 2025

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased seri onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout seri && git pull --rebase)

@cyyever
Copy link
Collaborator Author
cyyever commented Feb 4, 2025

@albanD The suggestions were applied.

@cyyever cyyever requested a review from albanD February 5, 2025 04:02
@cyyever
Copy link
Collaborator Author
cyyever commented Feb 5, 2025

@albanD Reverted another change.

albanD
albanD reviewed Feb 5, 2025
View reviewed changes
Copy link
Collaborator
@albanD albanD left a comment

Choose a reason for hiding this comment

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

Sounds ok for the rest, not sure if @mikaylagawarecki has any comment here.

@albanD
Copy link
Collaborator
albanD commented Feb 13, 2025

I'm still having a hard time convincing myself that this is safe... @r-barnes any opinion on this as our resident C++ expert?

@cyyever cyyever requested a review from r-barnes March 13, 2025 13:16
@cyyever cyyever force-pushed the seri branch 2 times, most recently from 1c02a31 to 2991664 Compare 8000 March 14, 2025 02:56
@r-barnes
Copy link
Contributor

@albanD - I can review, but if you're uncertain the best bet might be to ask for smaller diffs breaking out these changes so that the unsafe ones can be atomically reverted.

@cyyever
Copy link
Collaborator Author
cyyever commented Mar 19, 2025

@r-barnes @albanD I can divide it into smaller ones if necessary.

@cyyever cyyever changed the title Enable clang-tidy on caffe2/serialize/ [1/N] Enable clang-tidy on caffe2/serialize/ May 14, 2025
@cyyever
Copy link
Collaborator Author
cyyever commented May 14, 2025

@albanD @r-barnes I have split it into smaller ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albanD albanD albanD left review comments

@mikaylagawarecki mikaylagawarecki Awaiting requested review from mikaylagawarecki mikaylagawarecki is a code owner

@Skylion007 Skylion007 Awaiting requested review from Skylion007

@r-barnes r-barnes Awaiting requested review from r-barnes

Assignees
No one assigned
Labels
caffe2 open source topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cyyever @pytorchmergebot @albanD @r-barnes @pytorchbot
0