This is an archive of the discontinued LLVM Phabricator instance.

[Tooling] Pull #include manipulation code from clangFormat into libToolingCore.
ClosedPublic

Authored by ioeric on May 5 2018, 2:06 PM.

Diff Detail

Event Timeline

ioeric created this revision.May 5 2018, 2:06 PM
ioeric updated this revision to Diff 145384.May 5 2018, 2:10 PM
  • Revert unintended change.
ioeric updated this revision to Diff 146012.May 9 2018, 3:00 PM
  • Merged with origin/master
ioeric updated this revision to Diff 146042.May 9 2018, 5:56 PM
  • Merged with origin/master

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.
WDYT?

113

Can we move them in the current commit too?
IncludeCategoryManager does look like something that should not be in the public header.

128

Do we want const& here, i.e. have we accidentally lost &?

ioeric updated this revision to Diff 146339.May 11 2018, 9:23 AM
ioeric marked 3 inline comments as done.
  • Rebase on D46758
  • Addressed comments.

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.

ioeric updated this revision to Diff 146384.May 11 2018, 12:31 PM
  • Also moved yaml traits.
ioeric updated this revision to Diff 146385.May 11 2018, 12:33 PM
  • Revert commit on wrong branch.

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
1720

It seems Style.IncludeStyle should be in the other change that pulls out IncludeStyle from FormatStyle.
Or am I missing something?

1880

Here too. Shouldn't it be part of the other change?

ilya-biryukov accepted this revision.May 14 2018, 8:38 AM

LGTM

lib/Format/Format.cpp
1720

As chatted offline, we need this to pull out IncludeCategoryManager out of Format.cpp.
That should be the only change in this commit, other than moving the code around, so there's no need to pull it into a separate commit.

This revision is now accepted and ready to land.May 14 2018, 8:38 AM
ioeric updated this revision to Diff 146666.May 14 2018, 1:06 PM
  • Merged with origin/master
  • Make copy instead of taking references to ensure lifetime.
This revision was automatically updated to reflect the committed changes.