Page MenuHomePhabricator

Manikishan (Manikishan Ghantasala)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 9 2019, 10:44 AM (15 w, 15 h)

Recent Activity

Thu, Sep 12

Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

>>! In D64695#1667696, @MyDeveloperDay wrote:

Does this need landing? given that you have a number of patches in flight perhaps it would be good to request commit access

Thu, Sep 12, 6:40 AM · Restricted Project

Aug 22 2019

Manikishan updated the summary of D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Aug 22 2019, 3:49 AM · Restricted Project
Manikishan updated the summary of D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Aug 22 2019, 3:49 AM · Restricted Project
Manikishan updated the summary of D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Aug 22 2019, 3:49 AM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

I have removed the introduction to the NetBSDStyle, as there are some styles that are needed to implemented before launching the NetBSD style.

Aug 22 2019, 3:46 AM · Restricted Project

Aug 21 2019

Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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.

I do feel like there could be a documentation change missing really to explain to people how this really works and what if anything they have to change in their existing .clang-format files

Aug 21 2019, 12:50 PM · Restricted Project
Manikishan retitled D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category. from [clang-format] Added new style rule: SortNetBSDIncludes to [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Aug 21 2019, 12:22 PM · Restricted Project

Aug 6 2019

Manikishan added inline comments to D65648: [clang-format] Add support to SpacesBeforeTrailingComments to add spaces before Block comments..
Aug 6 2019, 12:05 PM · Restricted Project

Aug 2 2019

Manikishan created D65648: [clang-format] Add support to SpacesBeforeTrailingComments to add spaces before Block comments..
Aug 2 2019, 5:57 AM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Aug 2 2019, 12:25 AM · Restricted Project

Aug 1 2019

Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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

Aug 1 2019, 11:41 PM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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,

Let me rephrase - for the exiting .clang-formats, that don't currently specify SortPriority,
this introduction of SortPriority should not change the header handling.
Is that the case, and if so is there sufficient test coverage for that?

I got your idea now.
No, there is no test coverage for that case, and with the current patch they have to add SortPriority.
To avoid this shall I set SortPriority as Priority as default if it is not defined? I think that will fix the issue.

any reviews on it ?

That's sounds like it will work. Can you add some additional test cases around this in SortIncludesTest.cpp. Also, adding a test case specifically for sorting the NetBSD headers would be good.

Aug 1 2019, 10:38 AM · Restricted Project

Jul 29 2019

Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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,

Let me rephrase - for the exiting .clang-formats, that don't currently specify SortPriority,
this introduction of SortPriority should not change the header handling.
Is that the case, and if so is there sufficient test coverage for that?

I got your idea now.
No, there is no test coverage for that case, and with the current patch they have to add SortPriority.
To avoid this shall I set SortPriority as Priority as default if it is not defined? I think that will fix the issue.

Jul 29 2019, 9:51 PM · Restricted Project

Jul 18 2019

Manikishan added inline comments to D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jul 18 2019, 9:20 AM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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,

Let me rephrase - for the exiting .clang-formats, that don't currently specify SortPriority,
this introduction of SortPriority should not change the header handling.
Is that the case, and if so is there sufficient test coverage for that?

I got your idea now.
No, there is no test coverage for that case, and with the current patch they have to add SortPriority.
To avoid this shall I set SortPriority as Priority as default if it is not defined? I think that will fix the issue.

Jul 18 2019, 1:40 AM · Restricted Project

Jul 17 2019

Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 17 2019, 10:09 AM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

Is there sufficient test coverage as to what happens if SortPriority is not set?

Jul 17 2019, 10:04 AM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 17 2019, 9:49 AM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

Thanks! Can you update the documentation too?

Jul 17 2019, 7:55 AM · Restricted Project

Jul 16 2019

Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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.

Jul 16 2019, 8:24 PM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 16 2019, 4:37 PM · Restricted Project

Jul 15 2019

Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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 becomes a very powerful capability. There are many people who use clang-format for a "non" standard style source trees, and they might like to use such a capability themselves without having to inherit from NetBSD type.

I agree it will be complex, but that is a good justification for adding hard coded a NetBSD style.

Jul 15 2019, 12:43 PM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 15 2019, 12:26 PM · Restricted Project
Manikishan added inline comments to D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jul 15 2019, 12:23 PM · Restricted Project
Manikishan added inline comments to D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jul 15 2019, 12:23 PM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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 done by setting priorities in IncludeCategories, but here we have nearly 40+ cases and categories to hardcode due to complex interdependencies between their headers. So, I have added this style reducing the cases using regex. And if this is fully parameterised any OS related project can add their own header priorities.

Note that IncludeCategories is already a regex - https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Jul 15 2019, 9:31 AM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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?

Jul 15 2019, 8:57 AM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

Is this change that was meant?

Jul 15 2019, 8:45 AM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 15 2019, 5:48 AM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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?

Jul 15 2019, 2:48 AM · Restricted Project
Manikishan added inline comments to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 15 2019, 1:51 AM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 15 2019, 1:51 AM · Restricted Project

Jul 14 2019

Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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?

Jul 14 2019, 10:07 AM · Restricted Project
Manikishan added a comment to D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

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 which should be grouped together and pass them to regex searches which then sets up priority.
But I dob the feasibility of this approach. Will come up with one.

I guess its not completely clear how this differs from just using the IncludeCategories?

Jul 14 2019, 4:33 AM · Restricted Project

Jul 13 2019

Manikishan updated the summary of D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 13 2019, 5:38 AM · Restricted Project
Manikishan updated the diff for D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..

This is the squashed commit of both Adding NetBSD Style and adding sortNetBSDIncludes

Jul 13 2019, 5:36 AM · Restricted Project
Manikishan updated subscribers of D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 13 2019, 5:32 AM · Restricted Project
Manikishan created D64695: [clang-format] Modified SortIncludes and IncludeCategories to priority for sorting #includes within the Group Category..
Jul 13 2019, 5:32 AM · Restricted Project

Jun 26 2019

Manikishan updated the summary of D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 26 2019, 5:44 AM · Restricted Project

Jun 12 2019

Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 12 2019, 12:26 PM · Restricted Project

Jun 11 2019

Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 11 2019, 7:36 AM · Restricted Project
Manikishan retitled D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine from [clang-format] Added New Style Rule: BitFieldDeclsOnSeparateLines to [clang-format] Added New Style Rule: OnePerLineBitFieldDecl.
Jun 11 2019, 6:19 AM · Restricted Project
Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 11 2019, 5:30 AM · Restricted Project
Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.

Changed Style name to OnePerLineBitFieldDecl

Jun 11 2019, 5:24 AM · Restricted Project

Jun 10 2019

Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.

Updated unittest

Jun 10 2019, 10:47 AM · Restricted Project
Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.

Made some missing style modifications in the last revision

Jun 10 2019, 9:17 AM · Restricted Project
Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.

Added unittests and made the changes suggested by @mgorny

Jun 10 2019, 9:03 AM · Restricted Project
Manikishan updated the diff for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 10 2019, 8:35 AM · Restricted Project

Jun 9 2019

Manikishan updated the summary of D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 9 2019, 12:38 PM · Restricted Project
Manikishan updated the summary of D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 9 2019, 12:38 PM · Restricted Project
Manikishan updated the summary of D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 9 2019, 12:35 PM · Restricted Project
Manikishan updated the summary of D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 9 2019, 12:35 PM · Restricted Project
Manikishan updated the summary of D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 9 2019, 12:35 PM · Restricted Project
Manikishan updated the summary of D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 9 2019, 12:35 PM · Restricted Project
Manikishan added a reviewer for D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine: djasper.
Jun 9 2019, 12:33 PM · Restricted Project
Manikishan created D63062: [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
Jun 9 2019, 12:21 PM · Restricted Project