Geoffrey Martin-Noble via llvm-dev
2021-Dec-06 22:04 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
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/20211206/996aba6d/attachment.html>
Geoffrey Martin-Noble via llvm-dev
2021-Dec-06 22:09 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
And slightly different numbers (though I think it doesn't change any conclusions): $ git grep -E '}\s+//\s+namespace' | wc -l 14030 $ git grep -E '}\s+//\s+end\s+namespace' | wc -l 4185 On Mon, Dec 6, 2021 at 2:04 PM Geoffrey Martin-Noble <gcmn at google.com> 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/20211206/51830caf/attachment.html>
Philip Reames via llvm-dev
2021-Dec-06 22:59 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
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 <mailto: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 <mailto: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 <mailto:llvm-dev at lists.llvm.org> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > <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/20211206/914650cc/attachment.html>