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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
