Also pull #include related style out of FormatStyle as tooling::IncludeStyle.
Details
- Reviewers
ilya-biryukov - Commits
- rG7129e63bcce9: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.
rC332287: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.
rL332287: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.
Diff Detail
- Repository
- rC Clang
- Build Status
Buildable 18005 Build 18005: arc lint + arc unit
Event Timeline
This change is big enough to be somewhat hard to grasp.
Maybe we could split it into two, to make it easier for review?
I.e.
- Extraction of IncludeStyle into a separate subclass.
- Moving stuff around without other refactorings.
include/clang/Format/Format.h | ||
---|---|---|
1819 | for the behavior is a little confusing. Could we try elaborating on what 'behavior' is a bit more? E.g. the include manipulation is done via tooling::HeaderInclude, see its documentation for more details on how include insertion points are found and which edits are produced. | |
include/clang/Tooling/Core/HeaderIncludes.h | ||
23 | This struct looks big enough to justify putting it into a separate header. For the sake of keeping HeaderIncludes the first thing that people see when they open a file. | |
113 | Can we move them in the current commit too? | |
128 | Do we want const& here, i.e. have we accidentally lost &? |
The moving of IncludeStyle has been split into a separate patch D46758.
include/clang/Format/Format.h | ||
---|---|---|
1819 | Sure. Thanks for the suggestion! | |
include/clang/Tooling/Core/HeaderIncludes.h | ||
23 | Good point. Branched into D46758 | |
113 | Not yet... include sorting logics would also require refactoring in order to be moved from clang-format. |
This LG, presuming there are no semantic changes here, just moving the code around.
Also see the nits about IncludeStyle that seems to be in the wrong change...
include/clang/Tooling/Core/HeaderIncludes.h | ||
---|---|---|
113 | Ok, a FIXME mentioning that this class should be removed seems good enough. Thanks. | |
lib/Format/Format.cpp | ||
1702 | It seems Style.IncludeStyle should be in the other change that pulls out IncludeStyle from FormatStyle. | |
1862 | Here too. Shouldn't it be part of the other change? |
LGTM
lib/Format/Format.cpp | ||
---|---|---|
1702 | As chatted offline, we need this to pull out IncludeCategoryManager out of Format.cpp. |
- Merged with origin/master
- Make copy instead of taking references to ensure lifetime.
for the behavior is a little confusing. Could we try elaborating on what 'behavior' is a bit more? E.g. the include manipulation is done via tooling::HeaderInclude, see its documentation for more details on how include insertion points are found and which edits are produced.