Page MenuHomePhabricator

clang-format: Add ability to align assignment operators
ClosedPublic

Authored by matto1990 on Apr 3 2015, 8:30 AM.

Details

Reviewers
djasper
Summary

In Objective-C some style guides (including the one at my workplace) use a style where assignment operators are aligned, in an effort to increase code readability. This patch adds an option to the format library which allows this functionality. It is disabled by default for all the included styles, so it must be explicitly enabled.

The option will change code such as:

- (void)method {
    NSNumber *one = @1;
    NSNumber *twentyFive = @25;
}

to:

- (void)method {
    NSNumber *one        = @1;
    NSNumber *twentyFive = @25;
}

Diff Detail

Event Timeline

matto1990 updated this revision to Diff 23221.Apr 3 2015, 8:30 AM
matto1990 retitled this revision from to clang-format: Add ability to align assignment operators.
matto1990 updated this object.
matto1990 edited the test plan for this revision. (Show Details)
matto1990 added a subscriber: Unknown Object (MLST).
curdeius added inline comments.Apr 7 2015, 3:57 AM
lib/Format/WhitespaceManager.cpp
99

Shouldn't aligning of operators be done before the aligning of trailing comments?
Moreover, please add tests with aligned assignment operators and trailing comments as well as with operators and escaped newlines.

unittests/Format/FormatTest.cpp
8388

Add tests like

EXPECT_EQ(
            "int oneTwoThree = 123; // comment\n"
            "int oneTwo      = 12;  // comment",
            format(
                   "int oneTwoThree = 123;// comment\n"
                   "int oneTwo = 12;// comment", Alignment));

and

EXPECT_EQ(
            "#define MACRO                 \\\n"
            "int oneTwoThree = 123;        \\\n"
            "int oneTwo      = 12;         \\\n",
            format(
                   "#define MACRO \\"
                   "int oneTwoThree = 123;\\\n"
                   "int oneTwo = 12;\\", Alignment));
djasper added a subscriber: djasper.Apr 7 2015, 4:39 AM
djasper added inline comments.
docs/ClangFormatStyleOptions.rst
164

This is different from the comment below. Use docs/tools/dump_format_style.py.

include/clang/Format/Format.h
250

This comment is a bit brief. Add an example.

lib/Format/Format.cpp
423

This should be unnecessary as it is inherited from LLVMStyle.

482

This should be unnecessary as it is inherited from LLVMStyle.

lib/Format/WhitespaceManager.cpp
358

What does this do if an assignment needs to be wrapped, i.e.:

LooooooooooongType looooooooongVariable =
    someLooooooooooooooooongFunction();
int i = 1;

What does it do if there are multiple assignments in one statement, i.e.:

int i = 1, j = 2;
int abc = 3;

Does this align the assignments of statements other than DeclStmts? I.e. what is the result of:

int i = 1;
if (SomeType t = getSomething()) {}

int j = 7;
for (int k = 0; k < N; ++k) {}

Add tests.

unittests/Format/FormatTest.cpp
8349

Is there a particular reason, not to use verifyFormat everywhere?

matto1990 updated this revision to Diff 23452.Apr 8 2015, 4:11 PM

Fixed some of the smaller issues. There are still problems with alignment of escaped newlines which needs tests and fixes. I also need to add test cases for lines with assignment operations within a for loop and with multiple assignments on one line.

I've sent this diff in now to keep track of the changes made so far.

djasper added inline comments.Apr 8 2015, 10:46 PM
include/clang/Format/Format.h
250

Variable assignments sounds a bit vague. Is this for field assignments, too? Is this only meant to be in declarations or in all assignments? Does this only apply to "=" or also to the other assignment operators "+=", ...? I can answer these questions from the code, but a bit more detail might not hurt here and I am not yet sure the name AlignAssignmentOperators is ideal. Maybe AlignConsecutiveAssignments?

255

Please put the example as it would actually be formatted. There is too much whitespace here.

lib/Format/WhitespaceManager.cpp
358

The multi-line behavior of this (i.e. when a single declaration spans more than one line) is really interesting. Yet, there are no tests for it.

406

In LLVM style, we usually don't use {} for single-statement ifs.

unittests/Format/FormatTest.cpp
8336

Just thought of one more thing: Default parameters.

What happens to:

void SomeFunction(int parameter = 0) {
  int i = 1;
}

Similarly, would we want to align:

void SomeFunction(int parameter = 1,
                  int i =         2) { ...

? (The latter should definitely be another patch).

8341

I still think verifyFormat would be preferable for many of those, especially all those not testing the behavior around empty lines.

8379

Pure virtual methods might also be interesting, i.e.:

class C {
public:
  int i = 1;
  virtual void f() = 0;
};
matto1990 updated this revision to Diff 23658.Apr 12 2015, 12:37 PM

Updated to fix problems with escaled newlines. Still to work on:

  • Other assignment like +=
  • Don't align for default parameters
  • Switch to use verifyFormat when needed
  • Ignore assignments within for and if statements
  • Test multiple assigments on a single line
matto1990 updated this revision to Diff 23667.Apr 13 2015, 3:36 AM

Completed:

  • Align other assignments such as +=
  • Switch to using verifyFormat when possible
  • Test multiple assignments on a single line

Still to do:

  • Don't align for default parameters
  • Ignore assignments within for and if statements

I'm unsure what the best way to detect those two cases are. I thought about checking that either the token before (or two before when a type is given) the first assignment on a line is at the start of the line. Would that be an acceptable solution?

I don't think that that would be an acceptable solution as types can be more complicated, e.g. they can be "const" or templated.

Also, you are still missing test-cases where the assignment statements are multi-line.

unittests/Format/FormatTest.cpp
8357

I don't know whether you'd really want to align the other assigments as well. If you do, wouldn't it be better to align on the "="?

8358–8359

Do you really think that this behavior is a good idea?

8396

Replace 23 by 234 or something..

@djasper Can you think of a solution for not aligning on default parameters and assignments in for loops? Could we check for all the keywords like for at the beginning of the line and ignore that line if one is found?

unittests/Format/FormatTest.cpp
8349

When I used it it seemed to remove some of the newlines between each "block" of statements, which was making the tests fail. I think it was something to do with the mess up function.

matto1990 updated this revision to Diff 23990.Apr 18 2015, 4:12 PM

Implemented tests for all cases brought up by reviewers. This should be working now!

I think this is taking great shape. Thanks for continuing to work on it!

lib/Format/WhitespaceManager.cpp
145

Please add some comments.

157

Maybe rewrite this whole thing (ll. 156-174) as:

if (Changes[i].NewLinesBefore != 0) {
  CurrentLine += Changes[i].NewlinesBefore;
  if (StartOfSequence > 0 &&
      (Changes[i].NewLinesBefore > 1 || !FoundAssignmentOnLine)) {
    alignConsecutiveAssignments(StartOfSequence, i, MinColumn);
    MinColumn = 0;
    StartOfSequence = 0;
  }
  FoundAssignmentOnLine = false;
  FoundLeftParenOnLine = false;
}
177

I think you can simplify this boolean expression by pulling out "Changes[i].Kind == tok::equal"

182–184

These three statements are used at least three times. Maybe pull out a local lambda?

unittests/Format/FormatTest.cpp
8403

Format these changes with clang-format (it will break before "Alignment").

8438

Add (and fix if you have ideas):

verifyFormat("int i = 1;\n"
             "SomeType a = SomeFunction(looooooooooooooooooooooongParameterA,\n"
             "                          loooooooooooooooooooooongParameterB);\n"
             "int j = 2;", Alignment);

If not, still add the test (as clang-format would currently format it, probably aligning the first two equals) and a FIXME.

matto1990 updated this revision to Diff 24557.Apr 28 2015, 9:20 AM

I've fixed all the changes you've asked for. Sorry for the long delay. I wasn't sure of the best way to add the comment as not of the other methods in the class are commented at all. If you have any suggestions let me know!

Sorry. Looks like I introduced a lot more changes in the FormatTest.cpp file by running clang-format on the entire thing. Hope that's not a problem!

I think this basically looks good. Don't worry about the test changes, they all look trivial.

Btw., do you have commit access?

lib/Format/WhitespaceManager.cpp
158

I think just using:

auto AlignSequence = [&] { ... };

is fine for this locally-scoped lambda. But I am not an expert using lambdas. AFAIU, at least the "mutable" should be unnecessary as it only influences parameters captured by copy and all of yours are by reference, I think.

165

I was thinking more of very high-level comments that help understand the code, e.g.:

// Walk through all of the changes and find sequences of "=" to align.
// To do so, keep track of the lines and whether or not an "=" was found on
// align. If a "=" is found on a line, extend the current sequence. If the current
// line cannot be part of a sequence, e.g. because there is an empty line
// before it or it contains non-assignments, finalize the previous sequence.
180–184

For me personally, this is too detailed and too much of a replication of the source code in words. But this is probably a matter of taste.

200

reflow.

215

nit: append a period.

matto1990 updated this revision to Diff 24612.Apr 29 2015, 4:14 AM

I've fixed the problem with the lambda and changed the comments completely. I wasn't happy with them for the same reasons you mentioned, so I stuck with just a single comment at the top of the method as you suggested.

I don't have commit access, this is my first contribution.

djasper edited reviewers, added: djasper; removed: klimek, curdeius.Apr 29 2015, 4:53 AM
djasper accepted this revision.Apr 29 2015, 6:10 AM
djasper edited edge metadata.

Submitted as r236100. Thank you for working on this!

This revision is now accepted and ready to land.Apr 29 2015, 6:10 AM
djasper closed this revision.Apr 29 2015, 6:10 AM