[PATCH] D87587: [clang-format][PR47290] Make one-line namespaces resistant to FixNamespaceComments, update documentation

Tue Sep 15 08:14:59 PDT 2020

MyDeveloperDay added a comment.

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 breaking 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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87587/new/

https://reviews.llvm.org/D87587