-
Notifications
You must be signed in to change notification settings - Fork 560
Rework clang-format include groups.
#9715
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
Rework clang-format include groups.
#9715
Conversation
zhanyong-wan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please conform to the Google style guide for include ordering: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
In particular:
In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test the stuff in dir2/foo2.h, order your includes as follows:
dir2/foo2.h.
A blank line
C system headers, and any other headers in angle brackets with the .h extension, e.g., <unistd.h>, <stdlib.h>, <Python.h>.
A blank line
C++ standard library headers (without file extension), e.g., <algorithm>, <cstddef>.
A blank line
Other libraries' .h files.
A blank line
Your project's .h files.
Separate each non-empty group with one blank line.
Within each section the includes should be ordered alphabetically.
| # 3. PyTorch headers: | ||
| # a) API headers: `#include <torch/*.h>` | ||
| # b) ATen headers: `#include <ATen/*.h>` | ||
| # c) c10 headers: `#include <c10/*.h>` | ||
| # | ||
| # Note that PyTorch headers should be included with angle-brackets so as to | ||
| # keep consistency with PyTorch source code. This should also prevent | ||
| # headers to be included twice because of different paths. | ||
| # | ||
| # Google C++ Style Guide dictates (3) and (4) should belong to the same | ||
| # group. However, this breaks PyTorch/XLA build. The problem is that both | ||
| # PyTorch and abseil-cpp (included by OpenXLA) define (among many) the | ||
| # `LOG()` macro. This leads to problems, such as OpenXLA functions calling | ||
| # PyTorch `LOG()` macro due to include order. | ||
| # | ||
| # In order to avoid such a problem, we make PyTorch into its own group, | ||
| # just after the system libraries. This works because including OpenXLA | ||
| # headers will redefine PyTorch `LOG()` macro, solving the problem above. | ||
| # This, however might fail one day, if PyTorch starts calling `LOG()` macro | ||
| # in one of the headers we include in PyTorch/XLA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to move PyTorch headers to their own group, right after the C++ system libraries. Otherwise, we would get a build error where some OpenXLA files would call the LOG() macro defined by PyTorch, rather than the one defined by abseil-cpp. See the comment above for more details.
This PR modifies
clang-formatconfiguration file, so that its include groups are tailored for PyTorch/XLA. In summary, the changes make it so includes in C++ files are grouped like:Key Changes:
.clang-formatso as to recognize the include groups mentioned aboveUpdate (Nov 18):
Update (Nov 18):
c10/util/logging_is_not_google_glog.handabseil-cppabsl/log/log.hdefine aLOG()macroLOG()is redefined by PyTorch (i.e. it's included afterabseil-cpplog file), and any of those OpenXLA files are included afterwards, they will use PyTorchLOG()definition rather thanabseil-cpp. Raising the error.