Skip to content

Conversation

@suo
Copy link
Member

@suo suo commented Dec 24, 2018

The PR clang-formats everything in torch/csrc/jit/ and adds it to the pre-commit hook.

Here is a list of non-mechanical changes:

  • I went over each file and fixed up whenever I could tell that clang-format was clobbering comment formatting.
  • Made the macros in register_prim_ops a little more clang-format friendly by omitting trailing commas
  • Refactored autodiff.cpp to use a helper class with explicit state rather than a bunch of capturing lambdas
  • Small improvements to the precommit hook clang-format

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Dec 24, 2018
@suo suo requested review from soumith and zdevito December 24, 2018 17:47
@suo suo force-pushed the suo/clang-format-world branch from 5dbf34f to 757f76f Compare December 24, 2018 18:13
@suo suo force-pushed the suo/clang-format-world branch from 757f76f to 6d592ff Compare December 24, 2018 18:30
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.

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

Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

As far as I understand we still don't enforce clang-format on all files, nor did we even let people write some PR agains the partial check. Maybe it's better to wait for results of that before we merge a 10k line patch that doesn't do anything?

@suo
Copy link
Member Author

suo commented Dec 26, 2018

The enforcement (such as it is) is done through the precommit hook. It's not hard enforcement, but I'm trying to get the codebase in a reasonable state before then.

I can do things more incrementally, but it seems like that will probably be more pain for everyone involved (lots of rebasing and merge conflicts rather than ripping the bandaid off when few people have outstanding PRs).

nor did we even let people write some PR agains the partial check.

Do you mean that we haven't tested the hook enough?

@suo suo changed the title [jit] clang format world [jit] clang format jit Dec 26, 2018
@suo
Copy link
Member Author

suo commented Dec 26, 2018

Spoke offline with @apaszke and we decided to go ahead with band-aid ripping, followed by a warning CI check as well

@suo suo deleted the suo/clang-format-world branch December 26, 2018 15:02
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants