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>
Geoffrey Martin-Noble via llvm-dev
2021-Dec-06 23:05 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
I'm referring specifically to this part> That said, we should update the coding guidelines to recommend the formatwhich 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/20211206/f2f7e0c0/attachment.html>
Salman Javed via llvm-dev
2021-Dec-07 09:32 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
On 12/6/21 12:47 PM, James Y Knight via llvm-dev wrote:> That said, we should update the coding guidelines to recommend the > format which clang-tidy emits -- just to make everyone's lives easier.As a newcomer to this project, this would make my life easier. Before submitting a patch for a review, I want to have some confidence that I haven't made some rookie mistake (missing namespace closing comments being a perfect example), so naturally I will run clang-tidy and clang-format. Then the reviewer can spend more time looking at the meat of my patch, rather than acting like a kind of human clang-format/clang-tidy pre-merge tool. If the coding guidelines document defaulted to clang-tidy's suggested namespace comment style, that would cause the least amount of confusion for me. I will follow that style for any new namespaces I introduce to the code base. I wouldn't go out and proclaim that all existing code that uses the other style ("end namespace clang") is now in direct violation of the guidelines and must be changed, though.