[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

MyDeveloperDay added a comment. Assuming this works and the other unit tests don't show issues then this LGTM. Please consider running this on your NetBSD code base before committing, if possible please also run on clang code based to ensure existing sorted headers aren't sorted unexpectedly.

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 212978. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: include/clang/Format/Format.h include/clang/Tooling/Inclusions/HeaderIncludes.h

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 212974. Manikishan added a comment. Added Tests for using and not using SortPriority Field, Now, even if the SortPriorityis not set it will be set to the value of Category by default Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1606256 , @rdwampler wrote: > In D64695#1605676 , @Manikishan > wrote: > > > In D64695#1590948 , @Manikishan > > wrote: > > > > > In

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

rdwampler added a comment. In D64695#1605676 , @Manikishan wrote: > In D64695#1590948 , @Manikishan > wrote: > > > In D64695#1589818 , @lebedev.ri > > wrote: > > > > > In

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1590948 , @Manikishan wrote: > In D64695#1589818 , @lebedev.ri > wrote: > > > In D64695#1589740 , @Manikishan > > wrote: > > > > > In

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1589818 , @lebedev.ri wrote: > In D64695#1589740 , @Manikishan > wrote: > > > In D64695#1589508 , @lebedev.ri > > wrote: > > > > > Is

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

lebedev.ri added a comment. In D64695#1589740 , @Manikishan wrote: > In D64695#1589508 , @lebedev.ri > wrote: > > > Is there sufficient test coverage as to what happens if `SortPriority` is > > not set? > > > If

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 210358. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: include/clang/Tooling/Inclusions/HeaderIncludes.h include/clang/Tooling/Inclusions/IncludeStyle.h

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1589508 , @lebedev.ri wrote: > Is there sufficient test coverage as to what happens if `SortPriority` is not > set? If SortPriority is not set, the Includes will be grouped without sorting, For example:

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 210353. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: include/clang/Tooling/Inclusions/HeaderIncludes.h include/clang/Tooling/Inclusions/IncludeStyle.h

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

lebedev.ri added a comment. Is there sufficient test coverage as to what happens if `SortPriority` is not set? Comment at: lib/Format/Format.cpp:455-456 Style.KeepEmptyLinesAtTheStartOfBlocks); -IO.mapOptional("BitFieldDeclarationsOnePerLine",

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1589497 , @rdwampler wrote: > Thanks! Can you update the documentation too? Thanks! So, is this Implementation fine? But there is a thing, The values which we give two Priority does not really matter they need to

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

rdwampler added a comment. Thanks! Can you update the documentation too? Comment at: lib/Tooling/Inclusions/HeaderIncludes.cpp:211 + Ret = 0; +} + return Ret; I think you can drop the else block and just return immediately from the for loop.

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In the following patch I have modified Include categories by adding a new field "sortInlcudes" which defines the priority of the sort. And priority field will now only be used for grouping. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 210218. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: include/clang/Tooling/Inclusions/HeaderIncludes.h include/clang/Tooling/Inclusions/IncludeStyle.h

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1586157 , @MyDeveloperDay wrote: > I appreciate what you've done to make this patch less duplicated code..but > now you've almost got to the point where being able to control it all via > configuration. > > I'm

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 209933. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: docs/ClangFormatStyleOptions.rst include/clang/Format/Format.h lib/Format/Format.cpp

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

MyDeveloperDay added a comment. I appreciate what you've done to make this patch less duplicated code..but now you've almost got to the point where being able to control it all via configuration. I'm with @rdwampler I think you should be able to extend the include categories...and then this

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

rdwampler added a comment. > Sorry, my mistake I meant that I have added Regex for priorities while > sorting and If I am not wrong I think IncludeCategories are used while > Regrouping after sorting the Includes. In addition to that in my case I have > to sort the includes In a particular

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1585835 , @lebedev.ri wrote: > In D64695#1585772 , @Manikishan > wrote: > > > In D64695#1585754 , @rdwampler > > wrote: > > > > > I

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

lebedev.ri added a comment. In D64695#1585772 , @Manikishan wrote: > In D64695#1585754 , @rdwampler wrote: > > > I am not quite sure why this change is required to sort the headers for > > NetBSD, you can set the

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1585754 , @rdwampler wrote: > I am not quite sure why this change is required to sort the headers for > NetBSD, you can set the priorities via `IncludeStyle.IncludeCategories`. Is > that not sufficient? It can be

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

rdwampler added a comment. I am not quite sure why this change is required to sort the headers for NetBSD, you can set the priorities via `IncludeStyle.IncludeCategories`. Is that not sufficient? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 209873. Manikishan added a comment. Is this change that was meant? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: docs/ClangFormatStyleOptions.rst

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 209814. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: docs/ClangFormatStyleOptions.rst include/clang/Format/Format.h lib/Format/Format.cpp

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. > I guess my comment was that you patche introduces a lot of code duplication, > could the small amount of code you added be say put into a lambda and past in > based on style? Yes, I will add that and submit a patch soon. Repository: rC Clang CHANGES SINCE

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added inline comments. Comment at: unittests/Format/SortIncludesTest.cpp:709 + "#include \"pathnames.h\"\n", + sort("#include \n" + "#include \n" MyDeveloperDay wrote: > should you add a test which has groups

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 209776. Manikishan marked an inline comment as done. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files: include/clang/Format/Format.h lib/Format/Format.cpp Index:

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

MyDeveloperDay added inline comments. Comment at: unittests/Format/SortIncludesTest.cpp:709 + "#include \"pathnames.h\"\n", + sort("#include \n" + "#include \n" should you add a test which has groups defined already, I'm

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

MyDeveloperDay added a comment. In D64695#1584740 , @Manikishan wrote: > In D64695#1584706 , @MyDeveloperDay > wrote: > > > There also seems like alot of duplication between the existing > > sortCppIncludes > > >

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan marked an inline comment as done. Manikishan added a comment. In D64695#1584706 , @MyDeveloperDay wrote: > There also seems like alot of duplication between the existing sortCppIncludes > > I think the only difference here is really just > >

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

MyDeveloperDay added a comment. There also seems like alot of duplication between the existing sortCppIncludes I think the only difference here is really just SmallVector Indices; SmallVector Includes_p; for (unsigned i = 0, e = Includes.size(); i != e; ++i) { unsigned pl =

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan added a comment. In D64695#1584630 , @MyDeveloperDay wrote: > Do you think there is anything about this algorithm that could be > parameterized so that other projects could utilize it? I think we can take input from users the header files

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

MyDeveloperDay added a comment. Do you think there is anything about this algorithm that could be parameterized so that other projects could utilize it? I guess its not completely clear how this differs from just using the IncludeCategories? Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan updated this revision to Diff 209685. Manikishan added a comment. This is the squashed commit of both Adding NetBSD Style and adding sortNetBSDIncludes Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 Files:

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

Manikishan created this revision. Manikishan added reviewers: cfe-commits, mgorny, christos, MyDeveloperDay. Herald added a subscriber: krytarowski. Herald added a project: clang. Manikishan added subscribers: mgorny, christos. This new Style rule is made as a part of adding support for NetBSD