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

[MyDeveloperDay via Phabricator via cfe-commits]

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