[PATCH] D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.



================
Comment at: include/clang/Format/Format.h:1818
 ///   from \p Code if it exists.
+/// See documentation for `tooling::HeaderIncludes` for the behavior.
 llvm::Expected<tooling::Replacements>
----------------
`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`.


================
Comment at: include/clang/Tooling/Core/HeaderIncludes.h:22
+
+struct IncludeStyle {
+  /// Styles for sorting multiple ``#include`` blocks.
----------------
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?


================
Comment at: include/clang/Tooling/Core/HeaderIncludes.h:112
+/// priorities for headers.
+/// FIXME(ioeric): move this class into implementation file when clang-format's
+/// include sorting functions are also moved here.
----------------
Can we move them in the current commit too?
`IncludeCategoryManager` does look like something that should not be in the public header.


================
Comment at: include/clang/Tooling/Core/HeaderIncludes.h:127
+
+  const IncludeStyle Style;
+  bool IsMainFile;
----------------
Do we want `const&` here, i.e. have we accidentally lost `&`?


Repository:
  rC Clang

https://reviews.llvm.org/D46496