Philip Reames via llvm-dev
2021-Dec-06 21:31 UTC
[llvm-dev] [docs][RFC] Style for "end namespace" comments
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 > 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/ebc9d8db/attachment-0001.html>
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>