Page MenuHomePhabricator

[clang-format] Add a new extra command line option for ide-specific formatting
Needs ReviewPublic

Authored by yvvan on Oct 10 2018, 4:35 AM.

Details

Summary

In order to use clang-format for formatting/indenting code in IDE we need to add some tricks which are never needed for clang-format itself.

Let's start with the option which keeps all existing line breaks when the flag is set. This option is available with extra parameter for reformat() function.

The next steps could be for example - adding dummy text to empty lines to indent them or applying format only for the code before the cursor (for incomplete code). There are much more tricks which could also be considered later.

Diff Detail

Event Timeline

yvvan created this revision.Oct 10 2018, 4:35 AM
krasimir requested changes to this revision.Oct 18 2018, 1:52 PM

In general there's a high bar for adding new style options to clang-format:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options

This revision now requires changes to proceed.Oct 18 2018, 1:52 PM

Do you know the better way to accomplish my aim than adding an option to libFormat? For example making a dependent library which serves a little different purpose than libFormat itself or something simpler?

Do you know the better way to accomplish my aim than adding an option to libFormat? For example making a dependent library which serves a little different purpose than libFormat itself or something simpler?

I don't know about the specific use-case, but in general a fail-safe way to force a line break in C++ code is to add an empty line comment on the preceding line.

I don't quite understand. What you are describing should already be the behavior with ColumnLimit=0 and I think your test should pass without the new option. Doesn't it?

yvvan added a comment.Jan 7 2019, 3:43 AM

I don't quite understand. What you are describing should already be the behavior with ColumnLimit=0 and I think your test should pass without the new option. Doesn't it?

As far as I see how it works column limit does not prevent the shrinking behavior.

Do you know the better way to accomplish my aim than adding an option to libFormat? For example making a dependent library which serves a little different purpose than libFormat itself or something simpler?

I don't know about the specific use-case, but in general a fail-safe way to force a line break in C++ code is to add an empty line comment on the preceding line.

This is close but will not work for the case when more than two lines are packed together. So it requires to put the empty comment on the each line ending which is not too nice (if i'm not missing something)

$ cat /tmp/test.cc
int foo(int a,
           int b) {
      f();
  }

$ clang-format -style="{ColumnLimit: 0}" /tmp/test.cc
int foo(int a,
        int b) {
  f();
}

Is this not what you want? If so, in what way?

yvvan added a comment.Jan 7 2019, 4:42 AM
$ cat /tmp/test.cc
int foo(int a,
           int b) {
      f();
  }
 
$ clang-format -style="{ColumnLimit: 0}" /tmp/test.cc
int foo(int a,
        int b) {
  f();
}
 

Is this not what you want? If so, in what way?

Ok, this might mean that my test is bad and does not test what I want (there are more cases which are not formatted the way I want with ColumnLimit: 0). But since the patch is never to be submitted I don't see a reason to improve it.

Without understanding this in more detail I do not know how to move forward with this. What this patch is describing is what should already be the case with ColumnLimit set to zero. If it isn't this might be a bug or there might be a different way to move forward. However, the flag as is does not make sense to me without more information.

yvvan added a comment.Jan 7 2019, 4:49 AM

@djasper
Ok, if there's a possible way to go forward I will find time to provide a better test which behaves differently depending on this check. Also my current patch is a bit bigger than this version.

yvvan updated this revision to Diff 181274.Jan 11 2019, 7:48 AM

The tests are improved - now they actually act differently with and without the introduced flag. Also few more cases are covered (see the second added test).

Ok, but this behavior is still intended. You are setting clang-format to a format where it is breaking after binary operators and then added a break before a binary operator. clang-format assumes that this is not intended and that you will want this cleaned up.

E.g.:

$ cat /tmp/format.cc
if ( a
      && b) {
}

$ clang-format -style="{ColumnLimit: 0, BreakBeforeBinaryOperators: All}" /tmp/format.cc
if (a
    && b) {
}

$ clang-format -style="{ColumnLimit: 0}" /tmp/format.cc
if (a && b) {
}

I believe that this behavior is right and we should not work around it with an additional flag.

yvvan updated this revision to Diff 194844.Apr 12 2019, 5:25 AM
yvvan retitled this revision from [clang-format] Introduce the flag which allows not to shrink lines to [clang-format] Create a new tool for IDEs based on clang-format.
yvvan edited the summary of this revision. (Show Details)
yvvan added a reviewer: arphaman.

New approach - title and summary are updated.

You may be interested in D60605, which is a related idea (adding incremental format-and-indent-on-type to clangd).

These are opposite extremes in some sense: this patch integrates deeply into clang-format and that patch entirely layers on top of it, by transforming the source code (in hacky ways, in some cases).

I think the cleanest approach is probably some combination: in particular a "preserve line break at point" primitive does seem useful, and even if source code transforms are used, putting a nice API on this in clang-format would be nice. (I'm not sure a separate command-line-tool is justified, though).

If you haven't run into these yet, here are some fun issues I ran into:

  • clang-format bails out when brackets aren't sufficiently matched, incremental formatting needs to work in such cases
  • incremental formatting sometimes wants to make edits both immediately before and after the cursor. tooling::Replacements can't represent this, they will be merged and destroy precise cursor position information.
  • user expectations when breaking between () are fairly clear. {} is the same when it acts as a list, but not when it acts as a block!
yvvan added a comment.Apr 25 2019, 6:05 AM

@sammccall

Having a separate tool is nice because it allows the client to make it plugable. clang-format sometimes changes options quite significantly and it can be nice if you have a choice which version to pick, otherwise it might be unable to read the configuration you have.

@sammccall

Having a separate tool is nice because it allows the client to make it plugable. clang-format sometimes changes options quite significantly and it can be nice if you have a choice which version to pick, otherwise it might be unable to read the configuration you have.

I second Sam's concerns about introducing a new tool.
Could you list the use-cases that clang-format-ide would cover that clang-format doesn't?
How is introducing a new tool different from adding a new formatting option to clang-format?

yvvan added a comment.May 9 2019, 4:44 AM

@ilya-biryukov
I don't really care how it's used from the tool side. I'm also fine to have a new option in clang-format itself. That's why this review is here - to ask for opinions. It's easy to remove that "ide" part from this patch and just add an option for clang-format instead.
Do you have any other remarks/concerns apart from that?

My feedback would be:

  • I definitely think more control over preserving line breaks would be useful. Actually preserving *blank* lines is an important property. If you see D60605, there are a lot of cases where clang-format wants to delete the line you just added. Maybe we can make MaxEmptyLinesToKeep: 9999999 work in more cases.
  • I'm not convinced you need/want to preserve *all* line breaks, isn't it just one? (where the user pressed enter)
  • It's unclear why "extraformattingoptions" is needed, rather than just the usual formatting options, or some other parameter. It would be nice to avoid a new library entry point (overload) if possible.
  • as far as command-line interface goes, this definitely feels more like a flag than a new tool
yvvan added a comment.May 9 2019, 11:00 AM

My feedback would be:

  • I definitely think more control over preserving line breaks would be useful. Actually preserving *blank* lines is an important property. If you see D60605, there are a lot of cases where clang-format wants to delete the line you just added. Maybe we can make MaxEmptyLinesToKeep: 9999999 work in more cases.
  • I'm not convinced you need/want to preserve *all* line breaks, isn't it just one? (where the user pressed enter)
  • It's unclear why "extraformattingoptions" is needed, rather than just the usual formatting options, or some other parameter. It would be nice to avoid a new library entry point (overload) if possible.
  • as far as command-line interface goes, this definitely feels more like a flag than a new tool

Thanks for the feedback!
'to preserve *all* line breaks, isn't it just one' - it's just much easier code wise, you don't need to take the current position into account. I can look how I can make it for only one line.
'unclear why "extraformattingoptions" is needed' - the idea behind is that we can add more ide-specific options later without introducing new reformat parameters.
I will definitely remove a separate tool, it seems nobody sees the need of such thing.

yvvan added a comment.May 11 2019, 6:45 AM

@sammccall
I can't avoid adding extra formatting options because my first attempt to introduce an ordinary clang-format option faced resistance because of not fitting the clang-format purpose to format files.

yvvan updated this revision to Diff 199150.May 11 2019, 11:42 AM

Sorry for unrelated formatting changes - that's clang-format's work :)

I've removed the extra executable.

I don't know how to force that behavior only for the given line (for that I need someone who can help) but in our usecase we should not care if we apply the rule to the whole file or only to the current line since we are only interested in one single Replacement which we can anyways filter afterwards.

yvvan updated this revision to Diff 199151.May 11 2019, 11:51 AM

Some misleading reformatting fixed.

yvvan retitled this revision from [clang-format] Create a new tool for IDEs based on clang-format to [clang-format] Add a new extra command line option for ide-specific formatting.May 11 2019, 11:59 AM
yvvan edited the summary of this revision. (Show Details)
MyDeveloperDay added a project: Restricted Project.Oct 11 2019, 2:44 AM
KyrBoh added a subscriber: KyrBoh.Fri, Oct 18, 7:29 AM