Page MenuHomePhabricator

[clang-format] Adding style option for absolute formatting
AbandonedPublic

Authored by acoomans on Jul 19 2018, 5:26 PM.

Details

Summary

When formatting only certain lines for a file (clang-format -lines=x:x), two types of indentation could be expected:

  • absolute: the indentation as if the whole file was formatted
  • relative: the indentation is relative to the previous line, even if those are not indented correctly and are not in the range of lines to format

The default behavior for clang-format is relative formatting; e.g: running clang-format -lines=3:3 on (note the extra white space in front of @optional):

@protocol A
 @optional
// comment
- (void)f;
@end
MACRO

would give:

@protocol A
 @optional
 // comment
 - (void)f;
 @end
 MACRO

This diff adds a style option to specify absolute indentation; continuing the example above, with the option set, running clang-format -lines=3:3 would give:

@protocol A
 @optional
// comment
- (void)f;
@end
MACRO

Initial bug report / feature request: https://bugs.llvm.org/show_bug.cgi?id=37975

Diff Detail

Event Timeline

acoomans created this revision.Jul 19 2018, 5:26 PM
acoomans edited the summary of this revision. (Show Details)Jul 19 2018, 5:28 PM
acoomans added a reviewer: cfe-commits.

An alternative to changing the whole behavior would be to make it a parameter / configuration option.

acoomans updated this revision to Diff 156571.Jul 20 2018, 12:49 PM
acoomans retitled this revision from [WIP] Change clang-format to absolute indentation to [clang-format] Adding style option for absolute formatting.
acoomans edited the summary of this revision. (Show Details)

Could you explain what problem this is fixing?

acoomans edited the summary of this revision. (Show Details)Jul 23 2018, 10:03 AM

@djasper I updated the description of the diff.

This fixes the issue of clang-format -lines=x:x not returning the same results as clang-format, while keeping the current behavior as default.

Also ping @jolesiak since he initially filed the report

In my opinion, this only addresses one edge case where clang-format -lines output is not identical with a full reformatting. I believe they cannot all usefully be avoided. As such, I am unsure that this option carries its weight of making the implementation more complex.
How often does this happen for you? Why can't you just format the additional incorrect line?

I don't know; I just picked a random bug from the Bugzilla to get myself familiarized with the LLVM codebase :)

@jolesiak?

Sorry for the delay, I'll comment on that tomorrow.

First of all, thanks, Arnaud, for looking into this.

Let's address the mentioned bug first.
Let's assume that we use IndentReference = Relative. I think that this particular formatting bug is local (i.e. can be solved while processing a @protocol block). Look at this example:

if (true) {
  int a = 42;
    int b = 42;
  int c = 42;
}
int d = 42;

When we run clang-format -lines=4:4 we get:

if (true) {
  int a = 42;
    int b = 42;
    int c = 42;
}
int d = 42;

Please note that neither } nor int d = 42 is indented.
But this is a different behavior from what we see after running clang-format -lines=3:3 on:

@protocol A
 @optional
// comment
- (void)f;
@end
MACRO

the output is:

@protocol A
 @optional
 // comment
 - (void)f;
 @end
 MACRO

Please note that @end is indented (has different indentation than @protocol; MACRO is indented as well).
To clarify why this is undesired, @end ends @protocol, not @optional. By the way, clang-format doesn't think that @end should match @optional, check the output of an extended example:

@protocol A
 @optional
 // comment
 - (void)f;
 @end
 MACRO
 @end

Second @end still doesn't match the @protocol.

I think it's perfectly fine to allow users to have custom indentations if they so choose. In this particular example, though, clang-format should stop indenting when it reaches @end, giving the following output:

@protocol A
 @optional
 // comment
 - (void)f;
@end
MACRO

Moving to the general case, I must say that introducing IndentReference = Relative/Absolute is a very interesting approach. I can imagine situations when somebody actually want to use IndentReference = Absolute. However, adding an additional option comes at quite big cost and I think that in this case it outweighs the benefits.
I think that a very good outcome could be to comment what is happening next to IndentTracker.adjustToUnmodifiedLine(TheLine); line. E.g. "We adjust an indentation to match the existing code to give users flexibility (instead of ignoring it). It's their responsibility to provide a correct formatting of lines they didn't initially change if these lines break formatting.".

Ok, so IIUC, understanding that @end effective ends a section much like "}" would address the currently observed problems?

Ok, so IIUC, understanding that @end effective ends a section much like "}" would address the currently observed problems?

I think so.

Thanks for the review. I’ll update when I’m back from vacation

Arnaud

acoomans abandoned this revision.Aug 9 2018, 2:24 PM

Changed approach based on feedback. New changeset: https://reviews.llvm.org/D50535