Page MenuHomePhabricator

[clang-format][PR47290] Make one-line namespaces resistant to FixNamespaceComments, update documentation
Needs RevisionPublic

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, it remains to ensure clang-format is idempotent.

Diff Detail

Event Timeline

kuzkry created this revision.Sep 13 2020, 4:00 AM
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.