ilya-biryukov added a comment. 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...
================ 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. ---------------- ioeric wrote: > ilya-biryukov wrote: > > Can we move them in the current commit too? > > `IncludeCategoryManager` does look like something that should not be in the > > public header. > Not yet... include sorting logics would also require refactoring in order to > be moved from clang-format. Ok, a FIXME mentioning that this class should be removed seems good enough. Thanks. ================ Comment at: lib/Format/Format.cpp:1702 // cases where the first #include is unlikely to be the main header. - IncludeCategoryManager Categories(Style, FileName); + tooling::IncludeCategoryManager Categories(Style.IncludeStyle, FileName); bool FirstIncludeBlock = true; ---------------- It seems `Style.IncludeStyle` should be in the other change that pulls out `IncludeStyle` from `FormatStyle`. Or am I missing something? ================ Comment at: lib/Format/Format.cpp:1862 StringRef FileName = Replaces.begin()->getFilePath(); - HeaderIncludes Includes(FileName, Code, Style); + tooling::HeaderIncludes Includes(FileName, Code, Style.IncludeStyle); ---------------- Here too. Shouldn't it be part of the other change? 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
