Carlos Galvez via llvm-dev
2021-Dec-07 10:06 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
In case it helps, this graph shows the actual distribution of both styles (in %), using the regex suggested by Geoffrey above. I personally thing this is just something clang-format should update automatically as files get modified - over time the style automatically becomes consistent. But that's another story, I'm mostly interested in how to improve the Coding Guidelines at this point. [image: llvm_style.png] Best regards, Carlos On Tue, Dec 7, 2021 at 12:05 AM Geoffrey Martin-Noble <gcmn at google.com> wrote:> I'm referring specifically to this part > > > That said, we should update the coding guidelines to recommend the > format which clang-tidy emits -- just to make everyone's lives easier. > > But I'm not sure the key question here is what James said :-P I agree that > reviewers should not pay any attention to this, but it would be nice if the > coding guidelines conformed to whatever clang-format does (which doesn't > even need to be explicit for this rule. It can just be a catch-all "do what > clang-format says"). I think the coding examples are non-normative, so > *shouldn't* be used as evidence of unrelated rules, but obviously people do > actually read them that way, so formatting them with clang-format seems > like a good idea. > > On Mon, Dec 6, 2021 at 2:59 PM Philip Reames <listmail at philipreames.com> > wrote: > >> Er, you and I are reading James' response very differently. >> >> To me, this topic does not matter. We do not need to enforce a rule on >> this. Reviewers should not be wasting time on this. >> >> Philip >> On 12/6/21 2:04 PM, Geoffrey Martin-Noble wrote: >> >> I think James basically said the opposite: that we should use whatever >> clang-format does and be done with it. Things that don't really matter are >> one of the best cases for a style guide (accompanied by tooling to do it >> automatically). Otherwise you get reviewers commenting as they did on >> Carlos' patch. I would recommend that we encourage people to not comment on >> such things, but having it tool enforced and consistent seems good. >> >> On Mon, Dec 6, 2021 at 1:31 PM Philip Reames via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> I agree with James. Both are reasonable, this doesn't really matter, we >>> don't have to pick and enforce one. >>> >>> Philip >>> On 12/6/21 12:47 PM, James Y Knight via llvm-dev wrote: >>> >>> Both styles accomplish the goal of annotating what namespace is being >>> closed -- and both are widely used within the codebase. So I think there's >>> not an intrinsic reason to prefer one over the other. They're both fine. >>> >>> That said, we should update the coding guidelines to recommend the >>> format which clang-tidy emits -- just to make everyone's lives easier. >>> >>> >>> >>> On Mon, Dec 6, 2021 at 3:03 PM Carlos Galvez via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> Hi, >>>> >>>> I was recently working on a patch and was asked during review to >>>> replace existing: >>>> "// end namespace clang" Style A >>>> with : >>>> "// namespace clang" Style B >>>> >>>> After that, I got interested to understand what the preferred style is, >>>> and found in the Coding Guidelines >>>> <https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions> >>>> that the style is actually Style A. >>>> >>>> On the other hand, clang-format will automatically enforce Style B on >>>> new code, via the FixNamespaceComments option, which is set to "true" for >>>> the LLVM style. clang-format will keep the Style A if it already exists, >>>> however. Most people using clang-format (outside LLVM) will probably be >>>> more familiar with Style B. >>>> >>>> Additionally, I have seen the following usage numbers in the repo: >>>> >>>> $ git grep '// >>>> >>>> * end' | wc -l 6724 $ git grep '//* namespace' | wc -l >>>> 14348 >>>> >>>> So Style B seems to be more adopted. Therefore I wanted to ask - should >>>> we update the Coding Guidelines to reflect this, and avoid these kinds of >>>> style discussions in code reviews? If so, what style should be preferred? I >>>> have a patch <https://reviews.llvm.org/D115115> for review and there >>>> seems to be a preference for keeping both styles. Regardless of the choice, >>>> I don't think this should lead to an urgent style change of the whole >>>> codebase. >>>> >>>> Looking forward to your feedback! >>>> >>>> Best regards, >>>> Carlos >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> llvm-dev at lists.llvm.org >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>> >>> _______________________________________________ >>> LLVM Developers mailing listllvm-dev at lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/2c01fc70/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm_style.png Type: image/png Size: 29802 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/2c01fc70/attachment-0001.png>
MyDeveloper Day via llvm-dev
2021-Dec-07 10:20 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
Nice chart.. (love a picture!) If we allow the systematic clang-formatting all the code, I'd be happy to make alterations to clang-format to allow a switch of // end of namspace X // end namespace X to // namespace X (which is currently allows any of all 3 forms) However at present we have enough problems getting the % of clang-formatted files in LLVM/Clang above 50% (including excluding lit tests), making such a change will likely reduce our overall clang-format clean status by 10% and we use these "clang-formatted" clean files as a regression suite for clang-format to ensure we are not breaking functionality. (which protects us and ultimately you). https://clang.llvm.org/docs/ClangFormattedStatus.html As for the RFC I totally agree with aligning the documentation to what clang-format does by default. MyDeveloperDay. On Tue, Dec 7, 2021 at 10:07 AM Carlos Galvez via llvm-dev < llvm-dev at lists.llvm.org> wrote:> In case it helps, this graph shows the actual distribution of both styles > (in %), using the regex suggested by Geoffrey above. I personally thing > this is just something clang-format should update automatically as files > get modified - over time the style automatically becomes consistent. But > that's another story, I'm mostly interested in how to improve the Coding > Guidelines at this point. > [image: llvm_style.png] > Best regards, > Carlos > > On Tue, Dec 7, 2021 at 12:05 AM Geoffrey Martin-Noble <gcmn at google.com> > wrote: > >> I'm referring specifically to this part >> >> > That said, we should update the coding guidelines to recommend the >> format which clang-tidy emits -- just to make everyone's lives easier. >> >> But I'm not sure the key question here is what James said :-P I agree >> that reviewers should not pay any attention to this, but it would be nice >> if the coding guidelines conformed to whatever clang-format does (which >> doesn't even need to be explicit for this rule. It can just be a catch-all >> "do what clang-format says"). I think the coding examples are >> non-normative, so *shouldn't* be used as evidence of unrelated rules, but >> obviously people do actually read them that way, so formatting them with >> clang-format seems like a good idea. >> >> On Mon, Dec 6, 2021 at 2:59 PM Philip Reames <listmail at philipreames.com> >> wrote: >> >>> Er, you and I are reading James' response very differently. >>> >>> To me, this topic does not matter. We do not need to enforce a rule on >>> this. Reviewers should not be wasting time on this. >>> >>> Philip >>> On 12/6/21 2:04 PM, Geoffrey Martin-Noble wrote: >>> >>> I think James basically said the opposite: that we should use whatever >>> clang-format does and be done with it. Things that don't really matter are >>> one of the best cases for a style guide (accompanied by tooling to do it >>> automatically). Otherwise you get reviewers commenting as they did on >>> Carlos' patch. I would recommend that we encourage people to not comment on >>> such things, but having it tool enforced and consistent seems good. >>> >>> On Mon, Dec 6, 2021 at 1:31 PM Philip Reames via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> I agree with James. Both are reasonable, this doesn't really matter, >>>> we don't have to pick and enforce one. >>>> >>>> Philip >>>> On 12/6/21 12:47 PM, James Y Knight via llvm-dev wrote: >>>> >>>> Both styles accomplish the goal of annotating what namespace is being >>>> closed -- and both are widely used within the codebase. So I think there's >>>> not an intrinsic reason to prefer one over the other. They're both fine. >>>> >>>> That said, we should update the coding guidelines to recommend the >>>> format which clang-tidy emits -- just to make everyone's lives easier. >>>> >>>> >>>> >>>> On Mon, Dec 6, 2021 at 3:03 PM Carlos Galvez via llvm-dev < >>>> llvm-dev at lists.llvm.org> wrote: >>>> >>>>> Hi, >>>>> >>>>> I was recently working on a patch and was asked during review to >>>>> replace existing: >>>>> "// end namespace clang" Style A >>>>> with : >>>>> "// namespace clang" Style B >>>>> >>>>> After that, I got interested to understand what the preferred style >>>>> is, and found in the Coding Guidelines >>>>> <https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions> >>>>> that the style is actually Style A. >>>>> >>>>> On the other hand, clang-format will automatically enforce Style B on >>>>> new code, via the FixNamespaceComments option, which is set to "true" for >>>>> the LLVM style. clang-format will keep the Style A if it already exists, >>>>> however. Most people using clang-format (outside LLVM) will probably be >>>>> more familiar with Style B. >>>>> >>>>> Additionally, I have seen the following usage numbers in the repo: >>>>> >>>>> $ git grep '// >>>>> >>>>> * end' | wc -l 6724 $ git grep '//* namespace' | wc -l >>>>> 14348 >>>>> >>>>> So Style B seems to be more adopted. Therefore I wanted to ask - >>>>> should we update the Coding Guidelines to reflect this, and avoid these >>>>> kinds of style discussions in code reviews? If so, what style should be >>>>> preferred? I have a patch <https://reviews.llvm.org/D115115> for >>>>> review and there seems to be a preference for keeping both styles. >>>>> Regardless of the choice, I don't think this should lead to an urgent style >>>>> change of the whole codebase. >>>>> >>>>> Looking forward to your feedback! >>>>> >>>>> Best regards, >>>>> Carlos >>>>> _______________________________________________ >>>>> LLVM Developers mailing list >>>>> llvm-dev at lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>> >>>> >>>> _______________________________________________ >>>> LLVM Developers mailing listllvm-dev at lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> llvm-dev at lists.llvm.org >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>> _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/de857a3f/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm_style.png Type: image/png Size: 29802 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/de857a3f/attachment.png>
Renato Golin via llvm-dev
2021-Dec-07 10:49 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
On Tue, 7 Dec 2021 at 10:07, Carlos Galvez via llvm-dev < llvm-dev at lists.llvm.org> wrote:> I personally thing this is just something clang-format should update > automatically as files get modified - over time the style automatically > becomes consistent. >No, clang-format shouldn't be changing the actual contents as much as it can, especially if clang-tidy does the same job. IIUC, James proposal was to match the documentation with whatever clang-tidy emits, whichever way it goes. The meaning of all versions are exactly the same and it doesn't really matter which way you have it. I echo some comments here that we should focus on bigger things. The only action I would take here is to update either the docs, or clang-tidy, to match each other and the general current preference, which seems to be "// namespace" (the blue bars). I would strongly discourage people patching existing sources to match the style just for the sake of style. This is one of those things that aren't worth having a patch just for this. cheers, --renato -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/c928d2fe/attachment-0001.html>