Skip to content

Large scale fix of python-related files in torch/csrc/ #14515

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
Closed
goldsborough wants to merge 2 commits into from
Closed

Large scale fix of python-related files in torch/csrc/ #14515

goldsborough wants to merge 2 commits into from

Conversation

@goldsborough
Copy link
Contributor

@goldsborough goldsborough commented Nov 29, 2018

No description provided.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Nov 29, 2018
goldsborough
goldsborough commented Nov 29, 2018
View reviewed changes
.clang-tidy Outdated
Copy link
Contributor Author

@goldsborough goldsborough Nov 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Let me try fixing this one

facebook-github-bot
facebook-github-bot reviewed Nov 29, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pietern
Copy link
Contributor

pietern commented Nov 29, 2018

This will conflict with the .clang-tidy mods I did in #14491.

@ezyang
Copy link
Contributor

ezyang commented Nov 29, 2018

@goldsborough I have some in flight diffs which would pass tidy after this change, but don't pass before. Should I NOLINT suppress, or just ship even with broken tidy?

@goldsborough
Copy link
Contributor Author

goldsborough commented Nov 29, 2018

@ezyang @pietern please NOLINT for now please (doesnt have to include the check name, I'll fix it after)

@goldsborough goldsborough force-pushed the clang-tidy-csrc branch 2 times, most recently from 62be89f to 4a5647b Compare November 30, 2018 01:48
@pietern
Copy link
Contributor

pietern commented Nov 30, 2018

Sorry @goldsborough, saw your message to late -- 220ce80 changed .clang-tidy as well.

apaszke
apaszke reviewed Nov 30, 2018
View reviewed changes
torch/csrc/jit/constants.cpp Outdated
Copy link
Contributor

@apaszke apaszke Nov 30, 2018

Choose a reason for hiding this comment

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

What was wrong with this std::move?

Copy link
Contributor Author

@goldsborough goldsborough Nov 30, 2018

Choose a reason for hiding this comment

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

The val is used after which clang-tidy does not like

torch/csrc/utils/invalid_arguments.cpp Outdated
Copy link
Contributor

@apaszke apaszke Nov 30, 2018

Choose a reason for hiding this comment

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

We could just replace those assignments with return

@goldsborough goldsborough force-pushed the clang-tidy-csrc branch 3 times, most recently from ae66147 to 251da24 Compare December 4, 2018 20:06
facebook-github-bot
facebook-github-bot reviewed Dec 4, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@goldsborough goldsborough force-pushed the clang-tidy-csrc branch 3 times, most recently from b51a0cf to b2d3ef7 Compare December 5, 2018 15:18
facebook-github-bot
facebook-github-bot reviewed Dec 7, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang ezyang added the merged label Jun 25, 2019
@colesbury colesbury mentioned this pull request Nov 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ezyang ezyang ezyang approved these changes

@pietern pietern Awaiting requested review from pietern

@teng-li teng-li Awaiting requested review from teng-li

+1 more reviewer

@apaszke apaszke apaszke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@goldsborough @pietern @ezyang @apaszke @facebook-github-bot