This is an archive of the discontinued LLVM Phabricator instance.

Paths

Table of Contentst

-
lib/Format/
-
Format/
3
WhitespaceManager.cpp
-
unittests/Format/
-
Format/
1
FormatTestComments.cpp

Differential D36967

[clang-format] Align trailing comments if ColumnLimit is 0
ClosedPublic

Authored by pboettch on Aug 21 2017, 8:32 AM.

Download Raw Diff

Details

Reviewers

djasper
krasimir

Commits

rGd3baa293ae74: Merging r311532: --------------------------------------------------------------…
rG4a9c26075181: [clang-format] Align trailing comments if ColumnLimit is 0
rC311532: [clang-format] Align trailing comments if ColumnLimit is 0
rL311573: Merging r311532:
rL311532: [clang-format] Align trailing comments if ColumnLimit is 0

Summary

ColumnLimit = 0 means no limit, so comment should always be aligned if requested. This was broken with

https://llvm.org/svn/llvm-project/cfe/trunk@304687

introduced via

https://reviews.llvm.org/D33830

and is included in 5.0.0-rc2. This commit fixes it and adds a unittest for this property.

Should go into clang-5.0 IMHO.

Diff Detail

Repository: rL LLVM

Event Timeline

pboettch created this revision.Aug 21 2017, 8:32 AM

Herald added a subscriber: klimek. · View Herald TranscriptAug 21 2017, 8:32 AM

djasper accepted this revision.Aug 21 2017, 8:42 AM

djasper added inline comments.

unittests/Format/FormatTestComments.cpp
2479	s/Colums/ColumnLimit/

This revision is now accepted and ready to land.Aug 21 2017, 8:42 AM

@pboettch: could you point me to information about how backporting patches to 5.0.0 works? This patch doesn't apply cleanly at head.

lib/Format/WhitespaceManager.cpp
477	If `Style.ColumnLimit == 0`, this subtraction overflows and gives a huge unsigned number. Could you please directly use `UINT_MAX` or something similar in that case?

This revision now requires changes to proceed.Aug 21 2017, 8:56 AM

pboettch added inline comments.Aug 21 2017, 9:15 AM

lib/Format/WhitespaceManager.cpp
477	I saw that it will overflow when adding the `== 0`. My idea was to simply restore the behavior as it was before your modification. It seems that `ColumnLimit=0` is not well handled by clang-format. There are a lot of cases where `ColumnLimit=10000` is not behaving the same way as `=0`.

Integrated requests made by @djasper and @krasimir .

Thank you! I'll ask around if this can be merged into 5.0.0.

This revision is now accepted and ready to land.Aug 21 2017, 9:52 AM

djasper added inline comments.Aug 21 2017, 12:28 PM

lib/Format/WhitespaceManager.cpp
477	That's very much intentional. ColumnLimit=10000 means (basically): Put as much as you can on one line. ColumnLimit=0 means: Try to stick with the original code's line breaks.

In D36967#847694, @krasimir wrote:

Thank you! I'll ask around if this can be merged into 5.0.0.

Since it's fixing a regression, merging it sounds good to me. Let me know when it lands.

@pboettch, do you have commit access or should I commit this patch for you?

@krasimir No I don't have commit right. Feel free to commit this change. Thanks.

Closed by commit rL311532: [clang-format] Align trailing comments if ColumnLimit is 0 (authored by krasimir). · Explain WhyAug 23 2017, 12:19 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

lib/

Format/

WhitespaceManager.cpp

3 lines

unittests/

Format/

FormatTestComments.cpp

7 lines

Diff 111987

lib/Format/WhitespaceManager.cpp

Context not available.
	continue;	continue;

	unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;	unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
	unsigned ChangeMaxColumn = Style.ColumnLimit >= Changes[i].TokenLength	unsigned ChangeMaxColumn =
		Style.ColumnLimit == 0 \|\| Style.ColumnLimit >= Changes[i].TokenLength
	? Style.ColumnLimit - Changes[i].TokenLength	? Style.ColumnLimit - Changes[i].TokenLength
		krasimirUnsubmitted Not Done Reply Inline Actions If `Style.ColumnLimit == 0`, this subtraction overflows and gives a huge unsigned number. Could you please directly use `UINT_MAX` or something similar in that case? krasimir: If `Style.ColumnLimit == 0`, this subtraction overflows and gives a huge unsigned number. Could…
		pboettchAuthorUnsubmitted Not Done Reply Inline Actions I saw that it will overflow when adding the `== 0`. My idea was to simply restore the behavior as it was before your modification. It seems that `ColumnLimit=0` is not well handled by clang-format. There are a lot of cases where `ColumnLimit=10000` is not behaving the same way as `=0`. pboettch: I saw that it will overflow when adding the `== 0`. My idea was to simply restore the behavior…
		djasperUnsubmitted Not Done Reply Inline Actions That's very much intentional. ColumnLimit=10000 means (basically): Put as much as you can on one line. ColumnLimit=0 means: Try to stick with the original code's line breaks. djasper: That's very much intentional. ColumnLimit=10000 means (basically): Put as much as you can on…
	: ChangeMinColumn;	: ChangeMinColumn;

Context not available.

unittests/Format/FormatTestComments.cpp

Context not available.
	"int k; // line longg long",	"int k; // line longg long",
	getLLVMStyleWithColumns(20)));	getLLVMStyleWithColumns(20)));

		// Always align if Colums = 0
		djasperUnsubmitted Not Done Reply Inline Actions s/Colums/ColumnLimit/ djasper: s/Colums/ColumnLimit/
		EXPECT_EQ("int i, j; // line 1\n"
		"int k; // line longg long",
		format("int i, j; // line 1\n"
		"int k; // line longg long",
		getLLVMStyleWithColumns(0)));

	// Align comment line sections aligned with the next token with the next	// Align comment line sections aligned with the next token with the next
	// token.	// token.
	EXPECT_EQ("class A {\n"	EXPECT_EQ("class A {\n"
Context not available.