MyDeveloperDay added a comment. In D64695#1584740 <https://reviews.llvm.org/D64695#1584740>, @Manikishan wrote:
> In D64695#1584706 <https://reviews.llvm.org/D64695#1584706>, @MyDeveloperDay > wrote: > > > There also seems like alot of duplication between the existing > > sortCppIncludes > > > > I think the only difference here is really just > > > > > > > > SmallVector<unsigned, 16> Indices; > > SmallVector<unsigned, 16> Includes_p; > > for (unsigned i = 0, e = Includes.size(); i != e; ++i) { > > unsigned pl = getNetBSDIncludePriority(Includes[i].Filename); > > Includes_p.push_back(pl); > > Indices.push_back(i); > > } > > > > > > vs the original > > > > SmallVector<unsigned, 16> Indices; > > for (unsigned i = 0, e = Includes.size(); i != e; ++i) > > Indices.push_back(i); > > > > > > plus way the sorting is performed, are we sure we couldn't have just made > > the original sorting more powerful based on style settings? > > > Does it mean that adding the priority to sort based on style? > like this: > > if (Style== NetBSD) > // set priority to netbsd's priority > else if(Style == X) > // set X's priority > > > I didn't want to mess up the original sorting and made up this patch, if we > have parameterise this solution, I will go for it. We should be able to make changes without fear of breaking other code, if we can't then we don't have enough tests 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? ================ Comment at: lib/Format/Format.cpp:1878 + SmallVector<unsigned, 16> Indices; + SmallVector<unsigned, 16> Includes_p; + for (unsigned i = 0, e = Includes.size(); i != e; ++i) { ---------------- Manikishan wrote: > MyDeveloperDay wrote: > > _p? I don't understand what it stands for? > > > > IncludesPriority? > Yes , it means priority I don't think Includes_p matches Clang naming conventions so I recommend changing it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64695/new/ https://reviews.llvm.org/D64695 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
