Page MenuHomePhabricator

[clang-format] Add the possibility to align assignments spanning empty lines or comments
Needs ReviewPublic

Authored by tinloaf on Sun, Jan 3, 4:33 AM.

Details

Summary

Currently, empty lines and comments break alignment of assignments on consecutive
lines. This makes the AlignConsecutiveAssignments option an enum that allows controlling
whether empty lines or empty lines and comments should be ignored when aligning
assignments.

Diff Detail

Event Timeline

tinloaf requested review of this revision.Sun, Jan 3, 4:33 AM
tinloaf created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSun, Jan 3, 4:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Two remarks, first: I made the change only for AlignConsecutiveAssignments, but I think it should be consistent for AlignConsecutiveMacros, AlignConsecutiveBitFields and AlignConsecutiveDeclarations, as well. If this change is accepted, I will make corresponding changes to the other three.

Second: Sorry if this change pops up twice. I botched the first revision by not understanding arcanist.

tinloaf updated this revision to Diff 314284.Sun, Jan 3, 7:43 AM

Fix formatting issue resulting from ancient clang-format.

MyDeveloperDay retitled this revision from [format] Add the possibility to align assignments spanning empty lines or comments to [clang-format] Add the possibility to align assignments spanning empty lines or comments.Sun, Jan 3, 9:22 AM
MyDeveloperDay added a reviewer: curdeius.
MyDeveloperDay added a project: Restricted Project.
MyDeveloperDay added a comment.EditedSun, Jan 3, 9:27 AM

by and large, this looks pretty good to me..lets have some other comment

clang/docs/ClangFormatStyleOptions.rst
198

Did you regenerate this or make the changes by hand?

you need to run clang/tools/dump_style.py

clang/include/clang/Format/Format.h
110
clang/lib/Format/WhitespaceManager.cpp
390

add "." at the end

MyDeveloperDay added inline comments.Sun, Jan 3, 9:30 AM
clang/unittests/Format/FormatTest.cpp
12581

can you add an example with a block comment that spans multiple lines

e.g.

int a = 5
/*
 * block comment
*/
int oneTwoThree = 123;
int a = 5
///
/// block comment
///
int oneTwoThree = 123;
tinloaf updated this revision to Diff 314293.Sun, Jan 3, 10:19 AM
tinloaf marked 3 inline comments as done.

Address MyDeveloperDay's review comments.

tinloaf marked an inline comment as done.Sun, Jan 3, 10:21 AM
tinloaf added inline comments.
clang/include/clang/Format/Format.h
110

Sorry - I messed up my first try using arcanist, now I just used 'git diff' and forgot about -U999999. Should be fixed in the newest diff.

MyDeveloperDay added inline comments.Sun, Jan 3, 12:26 PM
clang/include/clang/Format/Format.h
141

Is there a case for aligning over empty lines and comments?

int a           = 5;

/* comment */
int oneTwoThree = 123;
clang/lib/Format/Format.cpp
140

move true and false to the bottom and separate with

// For backward compatibility.

comment (see below)

clang/lib/Format/WhitespaceManager.cpp
365

could this be an enum?

enum {
    None
    AcrossEmptyLines,
    AcrossComments,
    AcrossEmptyLinesAndComments,
}
curdeius added inline comments.Sun, Jan 3, 12:39 PM
clang/lib/Format/WhitespaceManager.cpp
418–433

Nit: unnecessary parentheses around !AcrossEmpty.
Same around !(LineIsComment && AcrossComments).
Maybe you might factor out a bool variable for this condition?

tinloaf marked an inline comment as done.Sun, Jan 3, 12:46 PM
tinloaf added inline comments.
clang/include/clang/Format/Format.h
141

Not sure I understand what you mean. Currently, the Option AcrossComments includes 'across empty lines'. So, there currently is no case for "across comments, but not across empty lines". I'm not sure if that is really something people want to do. Do you think so? I can add it.

MyDeveloperDay added inline comments.Sun, Jan 3, 10:53 PM
clang/include/clang/Format/Format.h
141

I could see a case where you might want to begin a new "alignment group" by leaving a blank line.

/* align these 3 */
int a = 5;
/* align these 3 */
int b = 6;
/* align these 3 */
int c = 7;

/* align these 2 which are longer */
int d           = 5
/* align these 2 which are longer */
int oneTwoThree = 123;
tinloaf updated this revision to Diff 314361.Mon, Jan 4, 4:58 AM

Address reviewer comments, noteworthy changes:

  • Add an option to span alignment only across comments, not across newlines
  • Factor out spanning options to AlignTokens to an enum
tinloaf marked 5 inline comments as done.Mon, Jan 4, 5:01 AM
tinloaf added inline comments.
clang/include/clang/Format/Format.h
141

Yes, good point. I've added the option AlignAcrossEmptyLinesAndComments, and made AlignAcrossComments do what the name suggests.

clang/lib/Format/WhitespaceManager.cpp
365

Yes, that's probably cleaner, though that means I need a rather ugly switch statement to convert one enum into the other where AlignTokens is called. Maybe one could address this with some template magic, but that's probably not less ugly than the switch statement.

418–433

Good point. I factored this out into two booleans, which should improve readability.

tinloaf updated this revision to Diff 314377.Mon, Jan 4, 7:22 AM
tinloaf marked 3 inline comments as done.

Fix bogus default case reported by clang-tidy.

MyDeveloperDay accepted this revision.Mon, Jan 4, 8:31 AM

This LGTM, I'm not sure if others have any further comments

Ideally we should add a comment to the release notes, (which you could do via a separate NFC commit if you wanted)

This revision is now accepted and ready to land.Mon, Jan 4, 8:31 AM

Ideally we should add a comment to the release notes, (which you could do via a separate NFC commit if you wanted)

Thank, I'll do that. But before that, I will create a revision that adds the same feature for the other three alignment types.

HazardyKnusperkeks requested changes to this revision.Mon, Jan 4, 12:28 PM

Ideally we should add a comment to the release notes, (which you could do via a separate NFC commit if you wanted)

Thank, I'll do that. But before that, I will create a revision that adds the same feature for the other three alignment types.

I think it should happen in this revision so that it is atomically.

But I'm willing to be overruled.

Apart from the 2 comments and the release notes nice change! (And I really have to think about what my new setting will be.)

clang/include/clang/Format/Format.h
128

You are adding a Tab here, or do I misinterpret this? Shouldn't that be fixed by clang-format itself?

clang/lib/Format/WhitespaceManager.cpp
647

All other variables start with an upper case char, maybe we should keep it that way?

This revision now requires changes to proceed.Mon, Jan 4, 12:28 PM
tinloaf added inline comments.Mon, Jan 4, 3:22 PM
clang/include/clang/Format/Format.h
128

I'm not sure, honestly. I did auto-format everything using clang-format with the provided style. I think I know what you mean (the little double arrows here in the side-by-side view?), but if I open the raw diff ("View Options" above, "Show Raw File (Right)"), it does not seem to contain tabs. Also, the file on my disk does not contain tabs.

Thank you for submitting this patch?

clang/include/clang/Format/Format.h
128

@HazardyKnusperkeks to my knowledge, its not a tab

MyDeveloperDay added inline comments.Tue, Jan 5, 2:34 AM
clang/docs/ClangFormatStyleOptions.rst
198–199

As you plan to do the other cases won't this introduce

AlignBitFieldStyle
and
AlignMacroStyle

why not have 1 AlignConsecutiveStyle that can be used for all 3, then you don't need the other enum TokenAlignmentStyle which means you won't need to map from one to the other?

tinloaf marked 2 inline comments as done.Tue, Jan 5, 3:32 AM
tinloaf added inline comments.
clang/docs/ClangFormatStyleOptions.rst
198–199

Yes, it would. I was under the assumption that every multiple-choice option in the options should have its own enum, for type safety. But yes, that would be way more elegant (and make the diff much smaller). I'll change it!

clang/include/clang/Format/Format.h
128

Great, I'll mark this as done then.

MyDeveloperDay added inline comments.Tue, Jan 5, 3:50 AM
clang/docs/ClangFormatStyleOptions.rst
198–199

we don't make that distinction for those options that use true/false so I'm not sure why enums need to be any different

it may require some changes to the doc/tools/dump_style.py to ensure it gives meaningful documentation, but some consistency between the values might help a little (especially as we'd like to use the same AlignTokens algortihm)

You'd end up with 3 functions to map the various styles to you TokenAlignmentStyle anyway, and hence you'd lose the type safety

To be honest I get a bit annoyed at the use of "None" "Never" "false", "No" anyway, so I don't feel we are in a great place in terms of consistency.

tinloaf updated this revision to Diff 315664.Sun, Jan 10, 7:10 AM
tinloaf marked an inline comment as done.
  • Unify the type of the options for AlignConsecutive{BitFields, Declarations, Assignments, Macros}
  • Implement across-alignment for bit fields, declarations, assignments and macros
  • Add tests for across-alignment for bit fields, declarations, assignments and macros
  • Add changelog entry

Ideally we should add a comment to the release notes, (which you could do via a separate NFC commit if you wanted)

Thank, I'll do that. But before that, I will create a revision that adds the same feature for the other three alignment types.

I think it should happen in this revision so that it is atomically.

Done - this revision now also includes the functionality for bit fields, macros and declarations.

tinloaf marked 3 inline comments as done.Sun, Jan 10, 7:16 AM

This is now an all-in-one revision, including bit fields, assignments, declarations and macros. I did not reproduce the full "across empty lines, across comments, across empty lines *and* comments" test suite for all four of them, since they all use the same logic (and code, mostly). AlignConsecutiveAssignments is tested extensively, AlignConsecutiveBitFields and AlignConsecutiveDeclarations only have tests for AlignAcrossEmptyLinesAndComments (since they delegate to the same AlignTokens method), and AlignConsecutiveMacros has its own set of tests, since it uses a different implementation (of the same logic, basically).

clang/docs/ClangFormatStyleOptions.rst
198–199

I have unified the type of the style options. I think the solution for the documentation is Okay, though not ideal. Each of the options' docs now starts with an example including the relevant tokens (e.g., bit field ':'s for AlignConsecutiveBitFields), but the examples illustrating the individual options always use assignments. I'm not sure how I could work around this using only a single type.

tinloaf marked an inline comment as done.Sun, Jan 10, 7:17 AM
tinloaf updated this revision to Diff 315665.Sun, Jan 10, 7:20 AM

Fix merge conflict in the release notes.

MyDeveloperDay accepted this revision.Sun, Jan 10, 8:04 AM

This LGTM

Apart from my inline comment and the pre merge check this is superb. (I will not accept it, until we have reached a conclusion for the documentation.)

clang/docs/ClangFormatStyleOptions.rst
428

This are now the examples from aligning the assignment. We now know that the rules for the alignments are the same, but someone new to it, just reading the documentation may be puzzled.

As @MyDeveloperDay said, most likely the script for generation has to be adapted?

The question here is now doing it now, or in a different change? It should happen before LLVM 12 will be released.

Apart from my inline comment and the pre merge check this is superb. (I will not accept it, until we have reached a conclusion for the documentation.)

Regarding the pre-merge check: it is clang-format which failed. My local clang-format claims that the code is formatted according to the project's .clang-format (git-clang-format tells me that there are no modifications), but the clang-format on the server seems to disagree. I can of course just apply the supplied patch, but I would like to fix this so that it won't take me two tries in the future. Which version of clang-format is running on the build server? I'm running 12.0.0.

clang/docs/ClangFormatStyleOptions.rst
428

I agree that this is not a good solution. I can probably adapt the doc generation script (have not yet looked at it in depth), but I currently have no good idea how to put the information into the source - i.e., where the dump script should get the info from. Here is an idea, which doesn't strike me as very elegant:

As far as I can tell, the script first takes the documentation block of the defined symbol (e.g., AlignConsecutiveDeclarations), and if the type of that symbol is an enum, then appends a list of possible enum values together with the documentation block of the respective enum value.

My suggestion would be to introduce some kind of tag into the documentation block of the defined symbol ( DOC_IGNORE_ENUM or something?), that, if present, deactivates this behavior and only takes the documentation of the defined symbol. I would then have to hand-code the different possible values and the respective examples into the documentation blocks of AlignConsecutiveAssignments, AlignConsecutiveMacros, AlignConsecutiveBitFields and AlignConsecutiveDeclarations. The documentation for AlignConsecutiveStyle would be ignored.

Would that be a feasible solution?

tinloaf updated this revision to Diff 316068.Tue, Jan 12, 5:27 AM

Fixed formatting issue.

I think I would remove the code examples from the "AlignConsecutive style" to avoid confusion (that would be the first change)

then perhaps regenerate and update the documentation so we can then see, its seems most have a "code block", can the specific examples not go into that area?

And just have the enum explain its different configuration options?

/// \brief If ``true``, aligns consecutive C/C++ preprocessor macros.
///
/// This will align C/C++ preprocessor macros of consecutive lines.
/// Will result in formattings like
/// \code
///   #define SHORT_NAME       42
///   #define LONGER_NAME      0x007f
///   #define EVEN_LONGER_NAME (2)
///   #define foo(x)           (x * x)
///   #define bar(y, z)        (y + z)
/// \endcode
bool AlignConsecutiveMacros;

I think I would remove the code examples from the "AlignConsecutive style" to avoid confusion (that would be the first change)

then perhaps regenerate and update the documentation so we can then see, its seems most have a "code block", can the specific examples not go into that area?

And just have the enum explain its different configuration options?

/// \brief If ``true``, aligns consecutive C/C++ preprocessor macros.
///
/// This will align C/C++ preprocessor macros of consecutive lines.
/// Will result in formattings like
/// \code
///   #define SHORT_NAME       42
///   #define LONGER_NAME      0x007f
///   #define EVEN_LONGER_NAME (2)
///   #define foo(x)           (x * x)
///   #define bar(y, z)        (y + z)
/// \endcode
bool AlignConsecutiveMacros;

I think that's a good way to go.

HazardyKnusperkeks requested changes to this revision.Fri, Jan 15, 2:59 PM
This revision now requires changes to proceed.Fri, Jan 15, 2:59 PM
tinloaf updated this revision to Diff 317166.Sat, Jan 16, 4:33 AM

Fixed documentation for the AlignConsecutive* options. I have used the suggested approach: Remove all documentation from AlignConsecutiveStyle and instead put the documentation of the possible values and what they do in the AlignConsecutive* options' docs.

I had to make two small changes to the documentation dump script for this:

  • Non-documented options in enums are silently ignored instead of raising an error, since AlignConsecutiveStyle has non-documented options now.
  • Indentation in the code-block lines is preserved, so that we can have indented code blocks in the documentation of the possible values.
tinloaf marked an inline comment as done.Sat, Jan 16, 4:34 AM

Not what I had in mind, but for me that's okay. I can not say anything to the change of the script though.

What I hat in mind was explaining (maybe in length) the enum and for the alignment settings just show an example with None and one of the other options, while referring to the enum documentation for all options.

clang/include/clang/Format/Format.h
87

If you don't document this with /// you don't need to remove the error from the script, or am I mistaken?