Page MenuHomePhabricator

[clang-format][PR47290] Add ShortNamespaceLines format option
ClosedPublic

Authored by kuzkry on Sep 13 2020, 4:00 AM.

Details

Summary

clang-format documentation states that having enabled
FixNamespaceComments one may expect below code:

c++
namespace a {
foo();
}

to be turned into:

c++
namespace a {
foo();
} // namespace a

In reality, no "// namespace a" was added. The problem was too high
value of kShortNamespaceMaxLines, which is used while deciding whether
a namespace is long enough to be formatted.

As with 9163fe2, clang-format idempotence is preserved.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2020, 4:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kuzkry requested review of this revision.Sep 13 2020, 4:00 AM

This fixes https://bugs.llvm.org/show_bug.cgi?id=47290.

@Jake
I decided I would at least create this pull request so you can decide whether you want it or not.

kuzkry updated this revision to Diff 291466.Sep 13 2020, 10:45 AM

Adjusted tests IncludeFixerTest.cpp after CI failure.

kuzkry updated this revision to Diff 291468.Sep 13 2020, 11:25 AM

Formatted code after CI failure.

JakeMerdichAMD requested changes to this revision.Sep 13 2020, 2:20 PM
JakeMerdichAMD edited reviewers, added: MyDeveloperDay; removed: jmerdich.
JakeMerdichAMD added a subscriber: MyDeveloperDay.

After looking at more of the history (ie, the commit you referenced), I'd definitely be open to something like this, provided that it doesn't affect namespaces that reside completely on one line. Since it was mostly a clang-format limitation and relatively rare, I think we can change the default here, but that's not up to just me (+@MyDeveloperDay), and extra scrutiny is definitely required when changing existing tests.

In any case, can you also add the full diff context? It makes it easier for us to review.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
426–428

I strongly believe that these ones are not correct. Namespaces that are entirely on one line should never have a trailing comment, even if it has content in it.

A solution would also have to take into account whether future passes would split this onto separate lines (and thus have a different result after re-running clang-format), which was the reason for this limitation in the first place.

This revision now requires changes to proceed.Sep 13 2020, 2:20 PM

Thanks @JakeMerdichAMD for your quick feedback.

In any case, can you also add the full diff context? It makes it easier for us to review.

I'll do that starting from the next patch because I'm afraid if I resubmit the longer version of the same patch, your comment would be hidden or considered obsolete as it would refer to an older revision.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
426–428

You're absolutely right. Btw. do you happen to know which style option controls this behaviour that splits these namespaces onto separate lines?
Anyway, I'm going to look into this in a few days.

kuzkry retitled this revision from [clang-format] Make one-line namespaces resistant to FixNamespaceComments, update documentation to [clang-format][PR47290] Make one-line namespaces resistant to FixNamespaceComments, update documentation.Sep 14 2020, 1:21 AM
MyDeveloperDay requested changes to this revision.EditedSep 14 2020, 8:44 AM

I have a hard time when people change tests! just because one person wants them this way doesn't mean everyone will.

I am not sure about others but I'd be ok about seeing this as an option to override the value but not changing the default.

I like to think of unit tests as the "Beyonce Rule", "if you liked it you put a test on it!"... when people come along and say... "All these tests are wrong"... I start thinking... is this subjective or objective?

The flux in llvm alone will be massive

https://github.com/llvm/llvm-project/search?q=namespace+%7B+++%7D&unscoped_q=namespace+%7B+++%7D

This would be a substantially smaller diff if you were adding the option. (option/doc/test...)

Full context diff please too!

clang/lib/Format/NamespaceEndCommentsFixer.cpp
28

I think change this will break the world... why don't we expose this as an option and not change everything!

Hey @MyDeveloperDay, thanks for the review.

On the one hand, you're right, it's a breaking change and I dislike it too.
But please, reconsider because on the other hand:
a) adding a new option means increasing our maintenance cost by possibly adding a rarely-used switch (which I also dislike)
b) it's been a part of clang-format documentation since release 5 so we could make clang-format work as expected
c) this patch doesn't have to be applied to already released versions and could be merged with a new major release. I think people can expect certain things to work differently while changing major versions.
d) in general, people care about preserving their git history and they normally don't format all their code blindly. I think the most frequent use case it to format only adjacent parts of code.

So, I'd say both ways have their ups and downs.

@all
Still, so far I've got two change requests from you lot but they are mutually exclusive.
To sum it up, which do you prefer?
a) add a new option (@MyDeveloperDay's approach)
b) change kShortNamespaceMaxLines (my approach) assuming @JakeMerdichAMD's suggestions (non-empty one-line namespaces aren't given the end comment)

MyDeveloperDay added a comment.EditedSep 15 2020, 8:14 AM

Firstly let me say thank you for the patch, and taking the time to listen to the reviewers. Please don't be discouraged by my reply/opinon.

a) adding a new option means increasing our maintenance cost by possibly adding a rarely-used switch (which I also dislike)

I always here people complaining about this...but what is the cost to ALL the users that see formatting changes every release across all the projects that use clang-format. Its potentially massive. I'm not sure the cost is that high for us.

b) it's been a part of clang-format documentation since release 5 so we could make clang-format work as expected

Either the documentation isn't being read/understood/misinterpreted, or perhaps the person who wrote this wanted it like this, to not namespace comment small namespaces. Again who can tell, but in my view the precedent has been set for 6 releases and I'd be uncomfortable about changing it now without a get out clause if we were wrong,

if your view was correct then having an option and setting the default to the new value so people could set it back is a far better solution because it doesn't create an impasse for those that don't like your opinion of what is correct? but I think hundreds of projects will complain they have to change the default. (as many as complain they have to set it in the first place potentially)

c) this patch doesn't have to be applied to already released versions and could be merged with a new major release. I think people can expect certain things to work differently while changing major versions.

If its not good enough for trunk its not good enough for the next release in my view, there is never a good time to land something that causes a lot of flux. remember people massively lag the release number and often jump many version numbers, by which time going back and changing a default will be far too late.

I think people can expect certain things to work differently while changing major versions.

I think certain people expect things to work differently, others, easily an equal number expect it to just work as is, and don't want subjective changes.

d) in general, people care about preserving their git history and they normally don't format all their code blindly. I think the most frequent use case it to format only adjacent parts of code.

We should not make assumptions about what others care about.. IMHO this is not true, I work in an organization where any file that is committed MUST conform fully to clang-format resulting in failed phabricator builds (by design), every time we update clang-format we have to makes some alterations for bug fixes. But this change would likely cause a change in almost every file and the impact and cost would be massive to us alone let alone every project that might use it.

This is also what happens inside LLVM (in some sub-projects) where checking for clang-format status is actually part of what will be done via the build_bots. As soon as you check this in, you'll break LLVM, you need to be prepared for that!

I don't want to discourage you, and my opinion is as subjective as yours. But I'm afraid I won't be able to hit the accept button with the review in this form.

Feel free to reach out to other reviewers, mine is not the only opinion.

Ultimately thank you for the patch I think the idea behind it is a good change, I'd personally just want to give people the option, I see this as no different to the people who wanted Small Functions/If/Else blocks formatted differently if they were considered "Small"

Many thanks @MyDeveloperDay

Thanks @MyDeveloperDay, I appreciate your input in this review. You're a good reviewer. :)
Also, I think you've made some good points there and I'm going to turn this review into "non-breaking the world" one. I'll be back in a few days with a new diff.

Thanks @MyDeveloperDay, I appreciate your input in this review. You're a good reviewer. :)
Also, I think you've made some good points there and I'm going to turn this review into "non-breaking the world" one. I'll be back in a few days with a new diff.

Thank you for considering that, I'm pretty sure I for one will be happy to approve it in its new form. I think this has merits.

kuzkry updated this revision to Diff 312940.EditedDec 19 2020, 8:17 AM
kuzkry retitled this revision from [clang-format][PR47290] Make one-line namespaces resistant to FixNamespaceComments, update documentation to [clang-format][PR47290] Add MaxUnwrappedLinesForShortNamespace format option.
kuzkry edited the summary of this revision. (Show Details)

As proposed by @MyDeveloperDay, I added MaxUnwrappedLinesForShortNamespace option to control behaviour of FixNamespaceComments. This time I'm providing you with a full diff.
Btw. sorry for taking so long.

EDIT: git clang-format is unhappy but I think it's properly formatted. Please, run it locally to check what I mean.

MyDeveloperDay accepted this revision.Dec 19 2020, 9:28 AM

Thanks for making the changes, this LGTM

MyDeveloperDay added inline comments.Dec 21 2020, 2:21 AM
clang/docs/ClangFormatStyleOptions.rst
2790

This is quite a mouthful before this lands do you want to consider shortening it?

ShortNamespaceLength ?

I'm not sure non clang-formast developers will know what an UnwrappedLine is?

curdeius added inline comments.
clang/docs/ClangFormatStyleOptions.rst
2790

I agree that one needs to wrap its head around to digest this name. I'd vote for what @MyDeveloperDay proposed.

HazardyKnusperkeks added inline comments.
clang/docs/ClangFormatStyleOptions.rst
2790

Although I already made some changes inside clang-format and single stepped through some formations, I still can not explain what those UnwrappedLines are.

curdeius added inline comments.Dec 21 2020, 2:14 PM
clang/docs/ClangFormatStyleOptions.rst
2790

Just found a precedence for this: https://clang.llvm.org/extra/clang-tidy/checks/llvm-namespace-comment.html. This clang-tidy check uses ShortNamespaceLines for the very same thing so maybe we should match?

kuzkry updated this revision to Diff 313218.Dec 21 2020, 4:34 PM
kuzkry retitled this revision from [clang-format][PR47290] Add MaxUnwrappedLinesForShortNamespace format option to [clang-format][PR47290] Add ShortNamespaceLines format option.

Renamed MaxUnwrappedLinesForShortNamespace to ShortNamespaceLines (thank you @MyDeveloperDay for the idea and @curdeius for mentioning clang-tidy). I haven't changed the description of this option so you might wanna complain on that. Tbh. English isn't my native language so I'll take your tips if you wanna help simplify it.

Luckily formatting is magically fixed and hopefully CI check's gonna be satisfied.

kuzkry marked 4 inline comments as done.Dec 21 2020, 4:39 PM
kuzkry marked an inline comment as done.

Apart from my comments, this looks good to me and I really wonder why it hasn't been pushed. (I would directly use it!)

clang/include/clang/Format/Format.h
3230

Could you sort this one alphabetically?

clang/lib/Format/Format.cpp
619

Dito

1002

Dito

curdeius requested changes to this revision.Feb 9 2021, 1:37 PM

Please regroup tests and add release notes.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
1126

This could be tested together with another test.

1129

In general, I think you can merge some test cases to match the current testing "style".

1141

From the tests one could imply that 0 means "never add comments", or "add comments for non empty namespaces". Please add a test with at least one line in the namespace.

1161

You should check that Style.ShortNamespaceLines is 1 here.

This revision now requires changes to proceed.Feb 9 2021, 1:37 PM
kuzkry updated this revision to Diff 322880.EditedFeb 10 2021, 5:47 PM

Rebased to the tip of the main branch, sorted code alphabetically as suggested by @HazardyKnusperkeks and applied almost all fixes proposed by @curdeius (entry in release notes + some changes in unit tests)

kuzkry marked 3 inline comments as done.Feb 10 2021, 5:51 PM

Apart from my comments, this looks good to me and I really wonder why it hasn't been pushed. (I would directly use it!)

I don't know, maybe noone needed this except for us :D

clang/include/clang/Format/Format.h
3230

Oops, I just sed'ed through it when renaming and forgot to sort it. Thx :)

kuzkry marked an inline comment as done.Feb 10 2021, 5:52 PM
kuzkry marked 3 inline comments as done.Feb 10 2021, 6:05 PM

Please regroup tests and add release notes.

Regroup tests - by that I think you meant all your 4 remarks about the tests. So I've done 3/4 :D
Release notes - done

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
1126

I merged it with "OneUnwrappedLine_*" tests

1129

Sorry, I didn't understand what you meant so I skipped this one for now.

1161

My initial idea was to have a separated test that would validate ShortNamespaceLines defaulting to "1". If that test failed, someone would know why and they might think:
"Oh my, I think I broke something, let's see what. Oh, I see the test is called "DefaultsToOneUnwrappedLine" so I probably changed this default". Now, some example test, say "OneUnwrappedLine_AddsEndCommentForLongNamespace", does two things, which is a bit like trying to kill two birds with one stone making it, imho, a bit inferior.

Almost there. I hope I'm clear this time on what I'd like to see.
Thanks for the release notes update.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
1162

Nit.

1201–1206

What I meant by "merging the tests together" is just writing:

TEST_F(ShortNamespaceLinesTest,
       MultipleUnwrappedLine) {
  auto Style = getLLVMStyle();
  Style.ShortNamespaceLines = 2u;

  EXPECT_EQ(...);

  EXPECT_EQ(...);
}

This will get rid of duplicated setup (even if small), and, what's more important, show the behaviour with the same style on different cases.

Or even, but I'm not forcing you to do so, having a single test case for all ShortNamespaceLinesTest.* tests. It groups the test for the same feature together.

kuzkry updated this revision to Diff 323289.Feb 12 2021, 4:17 AM
kuzkry marked 2 inline comments as done.

Rebased to the tip of the main branch, merged similar tests (requested by @curdeius)

kuzkry marked 2 inline comments as done.Feb 12 2021, 4:22 AM
kuzkry added inline comments.
clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
1162

Well, instead of using comments or giving it a separate test, I wanted to have a nice self-explanatory name.

Although I would put it in one test, this is fine by me.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
1201–1206

I would even go further and put the 0, 1, and 2 in one function.

Is this acceptable to the others? Do you need someone to push it?

Do you need someone to push it?

Yes, I need someone to do that. I don't have write permissions. I'm new here.

curdeius accepted this revision.Feb 28 2021, 1:44 AM

LGTM.

Do you need someone to push it?

Yes, I need someone to do that. I don't have write permissions. I'm new here.

Please state the name and mail for the commit. I will push it for you.

Do you need someone to push it?

Yes, I need someone to do that. I don't have write permissions. I'm new here.

Please state the name and mail for the commit. I will push it for you.

Name: Krystian Kuzniarek
Email: krystian.kuzniarek@gmail.com

This revision was not accepted when it landed; it landed in state Needs Review.Mar 1 2021, 12:29 PM
This revision was automatically updated to reflect the committed changes.
kuzkry added a comment.Mar 2 2021, 7:03 AM

Thank you for delivering it, @HazardyKnusperkeks.

One thing concerns me though. I'm not an author of changes done in clang/docs/ClangFormatStyleOptions.rst in lines 2372, 3069-3073, 3075, 3077, 3082-3086, 3088, 3093-3097, 3099, 3104-3108, and 3110-3111. Perhaps it's just a mistake (something might have slipped through git add). Please let me know.

Thank you for delivering it, @HazardyKnusperkeks.

One thing concerns me though. I'm not an author of changes done in clang/docs/ClangFormatStyleOptions.rst in lines 2372, 3069-3073, 3075, 3077, 3082-3086, 3088, 3093-3097, 3099, 3104-3108, and 3110-3111. Perhaps it's just a mistake (something might have slipped through git add). Please let me know.

I've just run dump_format_style.py, why the other lines were changed (or not correct before) I don't know, but this is the result of the official script.

If that bothers you I can revert and split it in two commits. But I don't think that's necessary as the real author of those lines is the script, not what's written in the git history.

kuzkry added a comment.Mar 9 2021, 4:01 PM

Thank you for delivering it, @HazardyKnusperkeks.

One thing concerns me though. I'm not an author of changes done in clang/docs/ClangFormatStyleOptions.rst in lines 2372, 3069-3073, 3075, 3077, 3082-3086, 3088, 3093-3097, 3099, 3104-3108, and 3110-3111. Perhaps it's just a mistake (something might have slipped through git add). Please let me know.

I've just run dump_format_style.py, why the other lines were changed (or not correct before) I don't know, but this is the result of the official script.

If that bothers you I can revert and split it in two commits. But I don't think that's necessary as the real author of those lines is the script, not what's written in the git history.

Oh, no need to do that. Thank you for your involvement in this review.