Policy for style guide: code comments #81452

Closed
opened 2020-10-05 13:52:28 +02:00 by Campbell Barton · 74 comments

This is a proposal for comment guidelines, this is intentionally minimal so everyone can stick to it easily.

This text would be used to update https:wiki.blender.org/wiki/Style_Guide/C_Cpp#CommentsNote that this is mostly formalizing what's already done, no big changes are expected as result of this proposal.//

Motivation

  • Consistent comments across the code-base are generally good for readability.
  • This makes running a spell checker on comments more useful/practical.
Checking spelling in comments can easily ignore most non-text if these conventions are followed.
  • This causes significantly fewer false positives when reporting badly spelled word (when running make check_spelling_c).
  • Editors may be configured to ignore non-English text too, e.g: tmp.png
  • Having a standard way to reference symbols means we can validate them,
seeing as comments becoming invalid or getting outdated often happens by accident.
  • Proper punctuation is more important now with clang-format wrapping comments.

Proposed Guidelines

Writing Style & What to Comment

  • Write in the third person perspective, to the point, using the same terminology as the code (think good quality technical documentation).

  • Be sure to explain non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them.

NOTE: This could be split into a separate proposal if it raises too much discussion here.

Punctuation

Use proper sentences with capitalized words and a full-stop.

Acronyms

Acronyms should always be written in upper-case (write API not api).

Syntax for including code

Literal Strings (following doxygen/markdown)
Code or any text that isn't plain English should be surrounded by back-ticks, eg:

/* This comment includes the expression `x->y / 2` using back-ticks. */

Symbols (following doxygen)

References to symbols such as a function, structs, enum values... etc should start with a #.

e.g: /** Remove by #wmGroupType.type_update_flag. */

Tag usage

Tags should be formatted as follows:

  • TODO: body text.

Or optionally, some information can be included:

  • TODO(@Username): body text. unique user name from developer.blender.org.
  • TODO(T123): body text. linking to the task associated with the TODO.
  • TODO([D123](https://archive.blender.org/developer/D123)): body text. linking to the differential associated with the TODO.

Common tags.

  • NOTE
  • TODO
  • FIXME
  • WORKAROUND use instead of HACK.
  • XXX general alert, prefer one of the more descriptive tags (above) where possible.

Comments should describe the problem and how it may be fixed, not only flagging the issue.

Email Addresses

Email formatting should use angle brackets, matching git First Last <name@addr.com>.

C/C++ Comments

The current code-style says that:

It is preferred to use C-style comments in C++ code too.

I'd like to make this statement more definite, e.g:

C-style comments should be used in C++ code.

Adding dead code is discouraged. In some cases, however, having unused code is useful (gives more semantic meaning, provides reference implementation, ...). It is fine having unused coed in this cases. Use // for a single-line code, and #if 0 for multi-line code. And always explain what the unused code is about.

This is a proposal for comment guidelines, this is intentionally minimal so everyone can stick to it easily. This text would be used to update https:*wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments*Note that this is mostly formalizing what's already done, no big changes are expected as result of this proposal.// ## Motivation - Consistent comments across the code-base are generally good for readability. - This makes running a spell checker on comments more useful/practical. ``` Checking spelling in comments can easily ignore most non-text if these conventions are followed. ``` - This causes significantly fewer false positives when reporting badly spelled word (when running `make check_spelling_c`). - Editors may be configured to ignore non-English text too, e.g: ![tmp.png](https://archive.blender.org/developer/F8957736/tmp.png) - Having a standard way to reference symbols means we can validate them, ``` seeing as comments becoming invalid or getting outdated often happens by accident. ``` - Proper punctuation is more important now with clang-format wrapping comments. ---- ## Proposed Guidelines ### Writing Style & What to Comment - Write in the third person perspective, to the point, using the same terminology as the code *(think good quality technical documentation).* - Be sure to explain non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them. NOTE: This could be split into a separate proposal if it raises too much discussion here. ### Punctuation Use proper sentences with capitalized words and a full-stop. ### Acronyms Acronyms should always be written in upper-case (write `API` not `api`). ### Syntax for including code **Literal Strings [(following doxygen/markdown)](https://www.doxygen.nl/manual/markdown.html#mddox_code_spans)** Code or any text that isn't plain English should be surrounded by back-ticks, eg: ``` /* This comment includes the expression `x->y / 2` using back-ticks. */ ``` **Symbols [(following doxygen) ](https://www.doxygen.nl/manual/autolink.html#linkother)** References to symbols such as a function, structs, enum values... etc should start with a `#`. e.g: `/** Remove by #wmGroupType.type_update_flag. */` ### Tag usage Tags should be formatted as follows: - `TODO: body text.` Or optionally, some information can be included: - `TODO(@Username): body text.` unique user name from `developer.blender.org`. - `TODO(T123): body text.` linking to the task associated with the `TODO`. - `TODO([D123](https://archive.blender.org/developer/D123)): body text.` linking to the differential associated with the `TODO`. Common tags. - `NOTE` - `TODO` - `FIXME` - `WORKAROUND` use instead of `HACK`. - `XXX` general alert, prefer one of the more descriptive tags (above) where possible. Comments should describe the problem and how it may be fixed, not only flagging the issue. ### Email Addresses Email formatting should use angle brackets, matching git `First Last <name@addr.com>`. ### C/C++ Comments The current code-style says that: > It is preferred to use C-style comments in C++ code too. I'd like to make this statement more definite, e.g: C-style comments should be used in C++ code. Adding dead code is discouraged. In some cases, however, having unused code is useful (gives more semantic meaning, provides reference implementation, ...). It is fine having unused coed in this cases. Use `//` for a single-line code, and `#if 0` for multi-line code. And always explain what the unused code is about.
Author
Owner

Added subscriber: @ideasman42

Added subscriber: @ideasman42
Author
Owner

Changed status from 'Needs Triage' to: 'Confirmed'

Changed status from 'Needs Triage' to: 'Confirmed'
Member

Added subscriber: @JacquesLucke

Added subscriber: @JacquesLucke

Added subscriber: @dr.sybren

Added subscriber: @dr.sybren

Literal Strings (following doxygen)
Symbols (following doxygen)

When these are mentioned on the wiki, please also add a link to the relevant Doxygen documentation. It just makes it so much easier to actually follow the rules.

Having a name is preferred.

Maybe even mandatory for new tags?

> Literal Strings (following doxygen) > Symbols (following doxygen) When these are mentioned on the wiki, please also add a link to the relevant Doxygen documentation. It just makes it so much easier to actually follow the rules. > Having a name is preferred. Maybe even mandatory for new tags?
Member

Added subscriber: @JulianEisel

Added subscriber: @JulianEisel
Member

Good guidelines.

It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out.

Good guidelines. It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out.
Member

Added subscriber: @EAW

Added subscriber: @EAW
Member

Email formatting should use angle brackets, matching git First List <name@addr.com>.

@ideasman42 List to Last?

> Email formatting should use angle brackets, matching git `First List <name@addr.com>`. @ideasman42 `List` to `Last`?
Author
Owner

In #81452#1028337, @dr.sybren wrote:

Literal Strings (following doxygen)
Symbols (following doxygen)

When these are mentioned on the wiki, please also add a link to the relevant Doxygen documentation. It just makes it so much easier to actually follow the rules.

Edited the proposal to link to doxygen, although doxygen syntax supports all sorts of things I'd rather not use in our plain (non-doxygen) comments.

Having a name is preferred.

Maybe even mandatory for new tags?

For TODO/FIXME/HACK ... etc. Having a name is good since there is something being left unresolved, knowing who to contact is useful. For NOTE/XXX, this is more of a general alert, so I don't think it's so important in those cases.
Of course git-blame can always be used, in practice though it can be tedious, especially when the git-blame doesn't lead directly back to the commit - for any reason.

I'd like to keep the guide-lines short & straightforward so it's not a hassle to follow/enforce, if others like to make names mandatory, I don't have a strong opinion against.

> In #81452#1028337, @dr.sybren wrote: >> Literal Strings (following doxygen) >> Symbols (following doxygen) > > When these are mentioned on the wiki, please also add a link to the relevant Doxygen documentation. It just makes it so much easier to actually follow the rules. Edited the proposal to link to doxygen, although doxygen syntax supports all sorts of things I'd rather not use in our plain (non-doxygen) comments. >> Having a name is preferred. > Maybe even mandatory for new tags? For `TODO/FIXME/HACK` ... etc. Having a name is good since there is something being left unresolved, knowing who to contact is useful. For `NOTE/XXX`, this is more of a general alert, so I don't think it's so important in those cases. *Of course git-blame can always be used, in practice though it can be tedious, especially when the git-blame doesn't lead directly back to the commit - for any reason.* I'd like to keep the guide-lines short & straightforward so it's not a hassle to follow/enforce, if others like to make names mandatory, I don't have a strong opinion against.
Author
Owner

In #81452#1028339, @JulianEisel wrote:
Good guidelines.

It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out.

Good point, I rather not make it part of this proposal, but it should be included on our code-style page.

> In #81452#1028339, @JulianEisel wrote: > Good guidelines. > > It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out. Good point, I rather not make it part of _this_ proposal, but it should be included on our code-style page.

In #81452#1028929, @ideasman42 wrote:

In #81452#1028339, @JulianEisel wrote:
It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out.

Good point, I rather not make it part of this proposal, but it should be included on our code-style page.

At that point we may also want to discuss where to put comments that document functions. Some people put them in at the declaration in the header, others put them at the definition. Might be nice to unify this too.

> In #81452#1028929, @ideasman42 wrote: >> In #81452#1028339, @JulianEisel wrote: >> It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out. > > Good point, I rather not make it part of _this_ proposal, but it should be included on our code-style page. At that point we may also want to discuss where to put comments that document functions. Some people put them in at the declaration in the header, others put them at the definition. Might be nice to unify this too.

Added subscriber: @brecht

Added subscriber: @brecht

I would add suggestions about the contents of comments, something like:

  • Write comments from a third person perspective, professional, to the point and using the same terminology as the code.
  • TODO comments should indicate what and how something may be fixed, rather than only flagging where there is code to be fixed.
  • Especially comment non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them.

Personally I am not a proponent of using names in comments like TODO(Name). If you need to ask someone for clarification most of the time, the comment may be incomplete. Even the author of the comment has likely forgotten what exactly the comment was about, or is no longer part of the project. To me it seems reasonable to use git blame for the cases where you need to clarification on a TODO comment, or any other comment or code.

If we keep this, the guidelines should mention what the name means. I wasn't sure if this was meant to indicate who to ask for more info, or who is expected to do the task, but from Campbell's comment it seems to be the former.

I would add suggestions about the contents of comments, something like: * Write comments from a third person perspective, professional, to the point and using the same terminology as the code. * TODO comments should indicate what and how something may be fixed, rather than only flagging where there is code to be fixed. * Especially comment non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them. Personally I am not a proponent of using names in comments like `TODO(Name)`. If you need to ask someone for clarification most of the time, the comment may be incomplete. Even the author of the comment has likely forgotten what exactly the comment was about, or is no longer part of the project. To me it seems reasonable to use git blame for the cases where you need to clarification on a TODO comment, or any other comment or code. If we keep this, the guidelines should mention what the name means. I wasn't sure if this was meant to indicate who to ask for more info, or who is expected to do the task, but from Campbell's comment it seems to be the former.
Member

In #81452#1029052, @brecht wrote:
If we keep this, the guidelines should mention what the name means. I wasn't sure if this was meant to indicate who to ask for more info, or who is expected to do the task, but from Campbell's comment it seems to be the former.

I agree that clarity would be good here. I thought it meant the latter, as in "Note to self: fix this later".

> In #81452#1029052, @brecht wrote: > If we keep this, the guidelines should mention what the name means. I wasn't sure if this was meant to indicate who to ask for more info, or who is expected to do the task, but from Campbell's comment it seems to be the former. I agree that clarity would be good here. I thought it meant the latter, as in "Note to self: fix this later".
Author
Owner

In #81452#1029027, @dr.sybren wrote:

In #81452#1028929, @ideasman42 wrote:

In #81452#1028339, @JulianEisel wrote:
It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out.

Good point, I rather not make it part of this proposal, but it should be included on our code-style page.

At that point we may also want to discuss where to put comments that document functions. Some people put them in at the declaration in the header, others put them at the definition. Might be nice to unify this too.

This is already covered, from the wiki:

As for placement of documentation, we try to keep the comments close to the code. Terse, single line descriptions of functions in headers is fine, but for detailed explanations, add this directly about the code.

We had a problem where developers often forget to update more detailed comments in headers. Or comments would be added to both and get out of sync - giving us work to figure out which one was most up to date, if they should be manually merged.


In #81452#1029052, @brecht wrote:
I would add suggestions about the contents of comments, something like:

  • Write comments from a third person perspective, professional, to the point and using the same terminology as the code.
  • TODO comments should indicate what and how something may be fixed, rather than only flagging where there is code to be fixed.
  • Especially comment non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them.

+1 (although writing style, what to comment could be a bigger topic, this seems fine).

Personally I am not a proponent of using names in comments like TODO(Name). If you need to ask someone for clarification most of the time, the comment may be incomplete. Even the author of the comment has likely forgotten what exactly the comment was about, or is no longer part of the project. To me it seems reasonable to use git blame for the cases where you need to clarification on a TODO comment, or any other comment or code.

If we keep this, the guidelines should mention what the name means. I wasn't sure if this was meant to indicate who to ask for more info, or who is expected to do the task, but from Campbell's comment it seems to be the former.

There has been a trend to add the name of the person who adds the TODO/FIXME/XXX in the code. I had the impression this was being preferred.

My intention with this proposal was to formalize what's already being done (for the most part), so if this isn't agreed on, we could try to come to an agreement here.

While I don't feel strongly about it, I do like having names, mainly because it gives some context.

As for git-blame being a reasonable substitute - for comments more than a few years old - tracking the original commit can take a few minutes - so I'd only do that if I have a real need to know the author. Besides this, agree that asking the author for more info is hint that comment wasn't sufficient.

> In #81452#1029027, @dr.sybren wrote: >> In #81452#1028929, @ideasman42 wrote: >>> In #81452#1028339, @JulianEisel wrote: >>> It may be a bit unclear when to use doxygen vs. regular comments. Should we have a guideline for that? Even if we allow individual modules to opt-out. >> >> Good point, I rather not make it part of _this_ proposal, but it should be included on our code-style page. > > At that point we may also want to discuss where to put comments that document functions. Some people put them in at the declaration in the header, others put them at the definition. Might be nice to unify this too. This is already covered, from the wiki: > As for placement of documentation, we try to keep the comments close to the code. Terse, single line descriptions of functions in headers is fine, but for detailed explanations, add this directly about the code. We had a problem where developers often forget to update more detailed comments in headers. Or comments would be added to both and get out of sync - giving us work to figure out which one was most up to date, if they should be manually merged. ---- > In #81452#1029052, @brecht wrote: > I would add suggestions about the contents of comments, something like: > * Write comments from a third person perspective, professional, to the point and using the same terminology as the code. > * TODO comments should indicate what and how something may be fixed, rather than only flagging where there is code to be fixed. > * Especially comment non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them. +1 *(although writing style, what to comment could be a bigger topic, this seems fine).* > Personally I am not a proponent of using names in comments like `TODO(Name)`. If you need to ask someone for clarification most of the time, the comment may be incomplete. Even the author of the comment has likely forgotten what exactly the comment was about, or is no longer part of the project. To me it seems reasonable to use git blame for the cases where you need to clarification on a TODO comment, or any other comment or code. > > If we keep this, the guidelines should mention what the name means. I wasn't sure if this was meant to indicate who to ask for more info, or who is expected to do the task, but from Campbell's comment it seems to be the former. There has been a trend to add the name of the person who adds the `TODO/FIXME/XXX` in the code. I had the impression this was being preferred. My intention with this proposal was to formalize what's already being done (for the most part), so if this isn't agreed on, we could try to come to an agreement here. While I don't feel strongly about it, I do like having names, mainly because it gives some context. As for git-blame being a reasonable substitute - for comments more than a few years old - tracking the original commit can take a few minutes - so I'd only do that if I have a real need to know the author. Besides this, agree that asking the author for more info is hint that comment wasn't sufficient.
Author
Owner

Updated the proposal based on feedback.

Added two notes:

  • What we consider good technical writing and what exactly to comment could be bigger topics, for the purpose of guidelines people can remember I'd rather keep them short, if this is a point of disagreement, I'd rather move that to a separate task.

  • We need to come to a decision regarding names, if TODO(Name): should be preferred/used-at-all. I've removed the text that names are preferred.

Personally I don't mind it but there are reasonable arguments for/against. We could avoid them for the same reason we removed names at the top of files, although it's less likely multiple people are involved contributing to one comment.
Updated the proposal based on feedback. Added two notes: - What we consider good technical writing and what exactly to comment could be bigger topics, for the purpose of guidelines people can remember I'd rather keep them short, if this is a point of disagreement, I'd rather move that to a separate task. - We need to come to a decision regarding names, if `TODO(Name):` should be preferred/used-at-all. I've removed the text that names are preferred. ``` Personally I don't mind it but there are reasonable arguments for/against. We could avoid them for the same reason we removed names at the top of files, although it's less likely multiple people are involved contributing to one comment.
Member

Added subscriber: @Harley

Added subscriber: @Harley
Member

All sounds good, but I have one dumb question. What exactly is meant by “name”? Username, first name, full name, email address?

The most obvious is username of course, but some of us have multiples just among b.o. systems. Some full-time devs have multiples. Even I go by “Harley” here but that is a different guy in some other places.

All sounds good, but I have one dumb question. What exactly is meant by “name”? Username, first name, full name, email address? The most obvious is username of course, but some of us have multiples just among b.o. systems. Some full-time devs have multiples. Even I go by “Harley” here but that is a different guy in some other places.
Author
Owner

We've been using first names which has been OK so far, however it's more correct to use usernames. This gets annoying for people with long usernames, so I'm not sure we want to enforce that.

The more I think about this the less I'm likeing including name - it gets messy, even now - when I see a comment from Lukas I'm not sure which one, over time it's only going to happen more.

We've been using first names which has been OK so far, however it's more _correct_ to use usernames. This gets annoying for people with long usernames, so I'm not sure we want to enforce that. The more I think about this the less I'm likeing including name - it gets messy, even now - when I see a comment from Lukas I'm not sure which one, over time it's only going to happen more.
Member

Could be a link to dev.b.o I suppose. https://developer.blender.org/p/harley/

Could be a link to dev.b.o I suppose. https://developer.blender.org/p/harley/
Member

Added subscriber: @ankitm

Added subscriber: @ankitm
Member

In #81452#1030338, @Harley wrote:
Could be a link to dev.b.o I suppose. https://developer.blender.org/p/harley/

The need for the full link everywhere can be removed by adding a note at one place which says use the Phabricator username.

For the TODO(Name): vs TODO:, how about TODO(T123): or TODO([D123](https://archive.blender.org/developer/D123)#456): ? If the comment is going to be longer than a few lines, it is possible that discussion/documentation is needed for that. So move it out of the code, to here on phabricator.
For smaller comments, use TODO(T0): to ease searching, and satisfy regex.

> In #81452#1030338, @Harley wrote: > Could be a link to dev.b.o I suppose. https://developer.blender.org/p/harley/ The need for the full link everywhere can be removed by adding a note at one place which says *use the Phabricator username*. For the `TODO(Name):` vs `TODO:`, how about `TODO(T123):` or `TODO([D123](https://archive.blender.org/developer/D123)#456):` ? If the comment is going to be longer than a few lines, it is possible that discussion/documentation is needed for that. So move it out of the code, to here on phabricator. For smaller comments, use `TODO(T0):` to ease searching, and satisfy regex.

Added subscriber: @Sergey

Added subscriber: @Sergey

Nice initiative, I like it!

HACK and XXX I would propose to discourage. HACK is ambiguous word, WORKAROUND is less ambiguous and describes actual intention of the comment better.
XXX is also ambiguous. It is either a NOTE or a WORKAROUND.

At that point we may also want to discuss where to put comments that document functions. Some people put them in at the declaration in the header, others put them at the definition. Might be nice to unify this too.

Not sure it's great to put everything to the same discussion. Adding more topics to the agenda only makes it harder to agree on it. Maybe we can start moving agreed-on points to the guide, keeping track of what is yet to be agreed on in the task itself (at the top).

Anyway, my opinion here is that header needs to explain what function does on API level. Comment in implementation file can go into implementation details. The reasoning behind this is because it is common to look for a function which does what you intend to, but it is not always clear from its naming. Constantly switching between header and implementation is annoying.

I think initial guideline about not using comments in the header is mainly caused by the "uber-headers", with all sort of APIs in a single file. Better would just be to split up such files.

Write comments from a third person perspective, professional, to the point and using the same terminology as the code.
TODO comments should indicate what and how something may be fixed, rather than only flagging where there is code to be fixed.
Especially comment non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them.

Fully agree with that.

About TODO(foo). The foo is supposed to gives clues where to find more information. Could be a name, email, task number. I don't think we should be having task assignments in the code.

Nice initiative, I like it! `HACK` and `XXX` I would propose to discourage. `HACK` is ambiguous word, `WORKAROUND` is less ambiguous and describes actual intention of the comment better. `XXX` is also ambiguous. It is either a `NOTE` or a `WORKAROUND`. > At that point we may also want to discuss where to put comments that document functions. Some people put them in at the declaration in the header, others put them at the definition. Might be nice to unify this too. Not sure it's great to put everything to the same discussion. Adding more topics to the agenda only makes it harder to agree on it. Maybe we can start moving agreed-on points to the guide, keeping track of what is yet to be agreed on in the task itself (at the top). Anyway, my opinion here is that header needs to explain what function does on API level. Comment in implementation file can go into implementation details. The reasoning behind this is because it is common to look for a function which does what you intend to, but it is not always clear from its naming. Constantly switching between header and implementation is annoying. I think initial guideline about not using comments in the header is mainly caused by the "uber-headers", with all sort of APIs in a single file. Better would just be to split up such files. > Write comments from a third person perspective, professional, to the point and using the same terminology as the code. > TODO comments should indicate what and how something may be fixed, rather than only flagging where there is code to be fixed. > Especially comment non-obvious algorithms, hidden assumptions, implicit dependencies, and design decisions and the reasons behind them. Fully agree with that. About `TODO(foo)`. The `foo` is supposed to gives clues where to find more information. Could be a name, email, task number. I don't think we should be having task assignments in the code.
Author
Owner
  • Replaced HACK with WORKAROUND.
  • Updated based on feedback. Having thought about policy for TODO(foo):, I'm preferring something less vague than first-names.

Anyway, my opinion here is that header needs to explain what function does on API level. Comment in implementation file can go into implementation details. The reasoning behind this is because it is common to look for a function which does what you intend to, but it is not always clear from its naming.
Constantly switching between header and implementation is annoying.

Rather move this into separate discussion.

- Replaced `HACK` with `WORKAROUND`. - Updated based on feedback. Having thought about policy for `TODO(foo):`, I'm preferring something less vague than first-names. ---- > Anyway, my opinion here is that header needs to explain what function does on API level. Comment in implementation file can go into implementation details. The reasoning behind this is because it is common to look for a function which does what you intend to, but it is not always clear from its naming. > Constantly switching between header and implementation is annoying. Rather move this into separate discussion.

+1 on the current state in the description.

+1 on the current state in the description.

Added subscriber: @mont29

Added subscriber: @mont29

+1 as well from me

+1 as well from me

C-style comments should be used in C++ code for plain-text comments. // should be used for commenting code.

This is ambiguous. All comments are plain text, as source code is plain text. All comment are commenting (on) code.

> C-style comments should be used in C++ code for plain-text comments. `//` should be used for commenting code. This is ambiguous. All comments are plain text, as source code is plain text. All comment are commenting (on) code.
Member

Added subscriber: @LazyDodo

Added subscriber: @LazyDodo
Member

Comments are comments, lets not try to give extra meaning to things that have none

// TODO: Do the thing.

and

/* TODO: Do the thing. */

are the same thing, lets not pretend otherwise and confuse any new developer coming into the blender project, feel free to pick a favorite and enforce as 'the style' for the whole project but lets not assign different meaning due to a different style being used.

Comments are comments, lets not try to give extra meaning to things that have none ``` // TODO: Do the thing. ``` and `/* TODO: Do the thing. */` are the same thing, lets not pretend otherwise and confuse any new developer coming into the blender project, feel free to pick a favorite and enforce as 'the style' for the whole project but lets not assign different meaning due to a different style being used.
Author
Owner

In #81452#1032443, @dr.sybren wrote:

C-style comments should be used in C++ code for plain-text comments. // should be used for commenting code.

This is ambiguous. All comments are plain text, as source code is plain text. All comment are commenting (on) code.

Replaced "Plain text" with "English text".

Edited (very minor update):

C-style comments should be used in C++ code for English text comments . // should be used for disabling source-code.


In #81452#1032682, @LazyDodo wrote:
Comments are comments, lets not try to give extra meaning to things that have none

// TODO: Do the thing.

and

/* TODO: Do the thing. */

are the same thing, lets not pretend otherwise and confuse any new developer coming into the blender project, feel free to pick a favorite and enforce as 'the style' for the whole project but lets not assign different meaning due to a different style being used.

The difference is you can always disable blocks of code with //. The same isn't true for /**/ as nesting isn't supported.

While there are ways around this - When Emacs has C-style comments set, it comments this...

  /* TODO: Do the thing. */
  a += 1;

...into this:

  /* /\* TODO: Do the thing. *\/ */
  /* a += 1; */

However I'd rather avoid escaping nested comments as it's not a well supported convention AFAIK.

> In #81452#1032443, @dr.sybren wrote: >> C-style comments should be used in C++ code for plain-text comments. `//` should be used for commenting code. > This is ambiguous. All comments are plain text, as source code is plain text. All comment are commenting (on) code. Replaced `"Plain text"` with `"English text"`. **Edited (very minor update):** > C-style comments should be used in C++ code for English text comments . // should be used for disabling source-code. ---- > In #81452#1032682, @LazyDodo wrote: > Comments are comments, lets not try to give extra meaning to things that have none > > ``` > // TODO: Do the thing. > ``` > and > > `/* TODO: Do the thing. */` > > are the same thing, lets not pretend otherwise and confuse any new developer coming into the blender project, feel free to pick a favorite and enforce as 'the style' for the whole project but lets not assign different meaning due to a different style being used. The difference is you can always disable blocks of code with `//`. The same isn't true for `/**/` as nesting isn't supported. While there are ways around this - When Emacs has C-style comments set, it comments this... ``` /* TODO: Do the thing. */ a += 1; ``` ...into this: ``` /* /\* TODO: Do the thing. *\/ */ /* a += 1; */ ``` However I'd rather avoid escaping nested comments as it's not a well supported convention AFAIK.
Member

A structure like

...
# endif```

to disable code blocks is already pretty common in the codebase so if it really is that big of an issue, we can just make that "the rule" for disabling blocks of code,  it suffers from none of these comment style issues. (cause lets be honest, "commenting" is just a means to and end there) bonus is we can easily write some tooling to keep an eye on how many of these zombie code blocks are in the codebase.

Ever since C99 both `*` and `/* */` are supported by both C and C++ , if we as a project like to enforce a comment style that be fine, consistency is a good thing to have! I honestly don't even care what we pick. But I very much would like to stay away from : "no, no this is a "text" comment, you're using the wrong style, you should have used `/* ... */` since we assigned special meaning to `*` and this is not it "

A structure like ```#if 0 ... # endif``` to disable code blocks is already pretty common in the codebase so if it really is that big of an issue, we can just make that "the rule" for disabling blocks of code, it suffers from none of these comment style issues. (cause lets be honest, "commenting" is just a means to and end there) bonus is we can easily write some tooling to keep an eye on how many of these zombie code blocks are in the codebase. Ever since C99 both `*` and `/* */` are supported by both C and C++ , if we as a project like to enforce a comment style that be fine, consistency is a good thing to have! I honestly don't even care what we pick. But I very much would like to stay away from : "no, no this is a "text" comment, you're using the wrong style, you should have used `/* ... */` since we assigned special meaning to `*` and this is not it "
Author
Owner

In #81452#1032909, @LazyDodo wrote:
A structure like

...
#endif``` 

to disable code blocks is already pretty common in the codebase so if it really is that big of an issue, we can just make that "the rule" for disabling blocks of code,  it suffers from none of these comment style issues. (cause lets be honest, "commenting" is just a means to and end there) bonus is we can easily write some tooling to keep an eye on how many of these zombie code blocks are in the codebase.

While using #if 0 is OK (even preferable for large blocks of code), there are some down-sides to making this a rule.

  • It's inconvenient to add/remove/toggle in most editors AFAIK.
  • They're overly verbose when commenting single lines.
  • They don't read very well when mixed with other pre-processor checks.

Ever since C99 both * and /* */ are supported by both C and C++ , if we as a project like to enforce a comment style that be fine, consistency is a good thing to have! I honestly don't even care what we pick. But I very much would like to stay away from : "no, no this is a "text" comment, you're using the wrong style, you should have used /* ... */ since we assigned special meaning to * and this is not it "

While I see your point that we could consider all comments equal and not differentiate between them. There are some advantages to doing this, also - you're suggestion is to remove a convention that's been applied to a lot of code over many years. While that doesn't make it right, it does mean we should have a good reason to change the existing convention.

Some advantages include:

  • We can easily switch to doxygen comments by changing /* text */ to /** text */.
  • It's easier to tell source-code/descriptive-text apart, examples from Blender's code:
  /* NOTE: commented out for follow constraint
   *
   *       If it's ever be uncommented watch out for BKE_curve_deform_coords()
   *       which used to temporary set CU_FOLLOW flag for the curve and no
   *       longer does it (because of threading issues of such a thing.
   */
  // if (cu->flag & CU_FOLLOW) {
            /* A disabled track will either be the track itself,
             * or one of the ones above it.
             *
             * If this is the top-most one, there is the possibility
             * that there is no active action. For now, we let this
             * case return true too, so that there is a natural way
             * to "move to an empty layer", even though this means
             * that we won't actually have an action.
             */
            // return (adt->tmpact != NULL);
> In #81452#1032909, @LazyDodo wrote: > A structure like > > ```#if 0 > ... > #endif``` > > to disable code blocks is already pretty common in the codebase so if it really is that big of an issue, we can just make that "the rule" for disabling blocks of code, it suffers from none of these comment style issues. (cause lets be honest, "commenting" is just a means to and end there) bonus is we can easily write some tooling to keep an eye on how many of these zombie code blocks are in the codebase. While using `#if 0` is OK *(even preferable for large blocks of code)*, there are some down-sides to making this a rule. - It's inconvenient to add/remove/toggle in most editors AFAIK. - They're overly verbose when commenting single lines. - They don't read very well when mixed with other pre-processor checks. ---- > Ever since C99 both `*` and `/* */` are supported by both C and C++ , if we as a project like to enforce a comment style that be fine, consistency is a good thing to have! I honestly don't even care what we pick. But I very much would like to stay away from : "no, no this is a "text" comment, you're using the wrong style, you should have used `/* ... */` since we assigned special meaning to `*` and this is not it " While I see your point that we could consider all comments equal and not differentiate between them. There are some advantages to doing this, also - you're suggestion is to remove a convention that's been applied to a lot of code over many years. While that doesn't make it right, it does mean we should have a good reason to change the existing convention. Some advantages include: - We can easily switch to doxygen comments by changing `/* text */` to `/** text */`. - It's easier to tell source-code/descriptive-text apart, examples from Blender's code: ``` /* NOTE: commented out for follow constraint * * If it's ever be uncommented watch out for BKE_curve_deform_coords() * which used to temporary set CU_FOLLOW flag for the curve and no * longer does it (because of threading issues of such a thing. */ // if (cu->flag & CU_FOLLOW) { ``` ``` /* A disabled track will either be the track itself, * or one of the ones above it. * * If this is the top-most one, there is the possibility * that there is no active action. For now, we let this * case return true too, so that there is a natural way * to "move to an empty layer", even though this means * that we won't actually have an action. */ // return (adt->tmpact != NULL); ```
Author
Owner

Since it seems like there is general agreement, I'll move this to the wiki unless there are any further replies here.

Since it seems like there is general agreement, I'll move this to the wiki unless there are any further replies here.

There are some points we agree on indeed.

However, as I was telling in blender.chat earlier this week I do not find the motivation behind // vs /**/ comments string enough. All the motivation I see so far is based on use of automated tools. Make tools smarter, don't make mankind to follow what Skynet tells to ;)

For one-liner comments the /**/ does not add information, only makes things more verbose in terms of both current state of the code and in terms of "noise" in any patch which tries to extend the comment:

  • When you go from single-line comment to multi-line one, you need to wrap previous */ to the new line.
  • If multiline comment has */ at its own line, this reduces available vertical space for the code.
  • If multiple comment has */ at the same line as last sentence of comment then it becomes noise in diff when comment is extended.
  • There might not be enough space for */ to close comment at the same line, resulting in a dedicated line which is there simply because of choice of comment.

In this regard even multi-line comments seems more tidy with // style comments.

Separation between /**/ for English and // for code does not work because /**/ is the proper way to deal with an unused arguments in C++.
There are less commonly places where /**/ is helpful as well. For example, to indicate that input parameter is const as far as logic goes, but it can not be marked so in the code because of legacy depths of Blender code which neglected const qualifier for a long time. Sure this is exotic, but still.

So as this specific part of proposal: I do not agree with it.
If majority of development team finds it super-useful, then I can live with it. If the opinion on a split I'd say we don't do it.

Another thing here is that I've asked you in blender.chat to send RFC mail to our mailing list, to make sure the entire development team is aware of this discussion. Please send such an e-mail.

There are some points we agree on indeed. However, as I was telling in blender.chat earlier this week I do not find the motivation behind `//` vs `/**/` comments string enough. All the motivation I see so far is based on use of automated tools. Make tools smarter, don't make mankind to follow what Skynet tells to ;) For one-liner comments the `/**/` does not add information, only makes things more verbose in terms of both current state of the code and in terms of "noise" in any patch which tries to extend the comment: * When you go from single-line comment to multi-line one, you need to wrap previous `*/` to the new line. * If multiline comment has `*/` at its own line, this reduces available vertical space for the code. * If multiple comment has `*/` at the same line as last sentence of comment then it becomes noise in diff when comment is extended. * There might not be enough space for `*/` to close comment at the same line, resulting in a dedicated line which is there simply because of choice of comment. In this regard even multi-line comments seems more tidy with `//` style comments. Separation between `/**/` for English and `//` for code does not work because `/**/` is the proper way to deal with an unused arguments in C++. There are less commonly places where `/**/` is helpful as well. For example, to indicate that input parameter is `const` as far as logic goes, but it can not be marked so in the code because of legacy depths of Blender code which neglected `const` qualifier for a long time. Sure this is exotic, but still. So as this specific part of proposal: I do not agree with it. If majority of development team finds it super-useful, then I can live with it. If the opinion on a split I'd say we don't do it. Another thing here is that I've asked you in blender.chat to send RFC mail to our mailing list, to make sure the entire development team is aware of this discussion. Please send such an e-mail.
Author
Owner

In #81452#1035430, @Sergey wrote:
There are some points we agree on indeed.

However, as I was telling in blender.chat earlier this week I do not find the motivation behind // vs /**/ comments string enough. All the motivation I see so far is based on use of automated tools. Make tools smarter, don't make mankind to follow what Skynet tells to ;)

For one-liner comments the /**/ does not add information, only makes things more verbose in terms of both current state of the code and in terms of "noise" in any patch which tries to extend the comment:

  • When you go from single-line comment to multi-line one, you need to wrap previous */ to the new line.

I assume you're talking about making comment blocks? I think this is a common enough convention (qt-creator, eclipse, emacs, vim can all be configured to do this - some even do by default IIRC).

  • If multiline comment has */ at its own line, this reduces available vertical space for the code.

Yes, we could avoid this more then we do (most new comments I keep on same line).

  • If multiple comment has */ at the same line as last sentence of comment then it becomes noise in diff when comment is extended.

While true, I don't find this a significant difference, less diff noise is something I'd like to favor. We could opt for using trailing */ on it's own line in that case, it's a trade-off.

  • There might not be enough space for */ to close comment at the same line, resulting in a dedicated line which is there simply because of choice of comment.

In this regard even multi-line comments seems more tidy with // style comments.

I don't find these arguments all that compelling. Yes the trailing */ uses a little space, but it's rarely an issue and we can wrap onto the next line in that case anyway.

If we use // everywhere we need blank-newlines between different comment blocks to show they aren't part of the same paragraphs, or part of code which also happens to be commented.

Separation between /**/ for English and // for code does not work because /**/ is the proper way to deal with an unused arguments in C++.
There are less commonly places where /**/ is helpful as well. For example, to indicate that input parameter is const as far as logic goes, but it can not be marked so in the code because of legacy depths of Blender code which neglected const qualifier for a long time. Sure this is exotic, but still.

This can be an exception to the rule, as with any C++ comments that require being inline.

So as this specific part of proposal: I do not agree with it.
If majority of development team finds it super-useful, then I can live with it. If the opinion on a split I'd say we don't do it.

Another thing here is that I've asked you in blender.chat to send RFC mail to our mailing list, to make sure the entire development team is aware of this discussion. Please send such an e-mail.

This was sent after our discussion: https://lists.blender.org/pipermail/bf-committers/2020-October/050688.html

> In #81452#1035430, @Sergey wrote: > There are some points we agree on indeed. > > However, as I was telling in blender.chat earlier this week I do not find the motivation behind `//` vs `/**/` comments string enough. All the motivation I see so far is based on use of automated tools. Make tools smarter, don't make mankind to follow what Skynet tells to ;) > > For one-liner comments the `/**/` does not add information, only makes things more verbose in terms of both current state of the code and in terms of "noise" in any patch which tries to extend the comment: > > * When you go from single-line comment to multi-line one, you need to wrap previous `*/` to the new line. I assume you're talking about making comment blocks? I think this is a common enough convention (qt-creator, eclipse, emacs, vim can all be configured to do this - some even do by default IIRC). > * If multiline comment has `*/` at its own line, this reduces available vertical space for the code. Yes, we could avoid this more then we do (most new comments I keep on same line). > * If multiple comment has `*/` at the same line as last sentence of comment then it becomes noise in diff when comment is extended. While true, I don't find this a significant difference, less diff noise is something I'd like to favor. We could opt for using trailing `*/` on it's own line in that case, it's a trade-off. > * There might not be enough space for `*/` to close comment at the same line, resulting in a dedicated line which is there simply because of choice of comment. > > In this regard even multi-line comments seems more tidy with `//` style comments. I don't find these arguments all that compelling. Yes the trailing `*/` uses a little space, but it's rarely an issue and we can wrap onto the next line in that case anyway. If we use `//` everywhere we need blank-newlines between different comment blocks to show they aren't part of the same paragraphs, or part of code which also happens to be commented. > Separation between `/**/` for English and `//` for code does not work because `/**/` is the proper way to deal with an unused arguments in C++. > There are less commonly places where `/**/` is helpful as well. For example, to indicate that input parameter is `const` as far as logic goes, but it can not be marked so in the code because of legacy depths of Blender code which neglected `const` qualifier for a long time. Sure this is exotic, but still. This can be an exception to the rule, as with any C++ comments that require being inline. > So as this specific part of proposal: I do not agree with it. > If majority of development team finds it super-useful, then I can live with it. If the opinion on a split I'd say we don't do it. > Another thing here is that I've asked you in blender.chat to send RFC mail to our mailing list, to make sure the entire development team is aware of this discussion. Please send such an e-mail. This was sent after our discussion: https://lists.blender.org/pipermail/bf-committers/2020-October/050688.html

In #81452#1032682, @LazyDodo wrote:
Comments are comments, lets not try to give extra meaning to things that have none

I fully agree.

In #81452#1032909, @LazyDodo wrote:
Ever since C99 both // and /* */ are supported by both C and C++

That's excellent, so let's stop calling one "C style" and the other "C++ style".

@Sergey also makes very good points, and I've run into those in practice. I've never run into the problem that I hard a hard time understanding that something I interpreted as English text was actually code, and that the style of comments would have helped me understand that.

In my opinion, we shouldn't even be committing commented-out code. It tends to rot and just sit there, and when the time comes to uncomment it, it may actually be doing the wrong thing. Just remove the code. If certain behaviour should be tested before the removal is permanent, that's what we have branches and custom test builds for. If the code should maybe be resurrected at some point, create a task for this on Phabricator that explains the situation, provides criteria for resurrecting the code, and reference the commit that removed it. That way anybody can verify the criteria and restore what's needed, without having to remember that somewhere in the code something was disabled.

> In #81452#1032682, @LazyDodo wrote: > Comments are comments, lets not try to give extra meaning to things that have none I fully agree. > In #81452#1032909, @LazyDodo wrote: > Ever since C99 both `//` and `/* */` are supported by both C and C++ That's excellent, so let's stop calling one "C style" and the other "C++ style". @Sergey also makes very good points, and I've run into those in practice. I've never run into the problem that I hard a hard time understanding that something I interpreted as English text was actually code, and that the style of comments would have helped me understand that. In my opinion, we shouldn't even be committing commented-out code. It tends to rot and just sit there, and when the time comes to uncomment it, it may actually be doing the wrong thing. Just remove the code. If certain behaviour should be tested before the removal is permanent, that's what we have branches and custom test builds for. If the code should maybe be resurrected at some point, create a task for this on Phabricator that explains the situation, provides criteria for resurrecting the code, and reference the commit that removed it. That way anybody can verify the criteria and restore what's needed, without having to remember that somewhere in the code something was disabled.

C-style comments:

  • ✓ Allows to more visually separate English from code
  • ✗ Verbose
  • ✗ Requires trade-off for */ on the same line vs. next line
  • ✗ Unavoidable in C++ code (violating use-for-English-only rule)

C++-style comments:

  • ✗ Looks the same for commented code and text
  • ✓ Brief
  • ✓ Intrinsically free from trade-offs mentioned for C-style, do not cause visual noise in diffs

Personally, I think I can easily tell commented code from commented text apart. And personally I am not supporting decisions based on current state of some specific automation tool.

For the examples you've showed where comment is followed with commented-out text I'd say go to the root of the issue, make this part of code actually helpful. As it states currently, commented out code does not bring information, the English comments are either to be eliminated or to be significantly shorten because they do not carry enough information.

More generally: I think the time which is being spent on this discussion is not being spent for good. If the goal is to improve comments in Blender, the focus must be on their content, not on decor.

This was sent some days ago: https://lists.blender.org/pipermail/bf-committers/2020-October/050688.html

Eeeh. Missed that in my inbox somehow.
Ok, so you've followed all known practices to get core team involved, without much feedback from the team given =\

C-style comments: - ✓ Allows to more visually separate English from code - ✗ Verbose - ✗ Requires trade-off for `*/` on the same line vs. next line - ✗ Unavoidable in C++ code (violating use-for-English-only rule) C++-style comments: - ✗ Looks the same for commented code and text - ✓ Brief - ✓ Intrinsically free from trade-offs mentioned for C-style, do not cause visual noise in diffs Personally, I think I can easily tell commented code from commented text apart. And personally I am not supporting decisions based on current state of some specific automation tool. For the examples you've showed where comment is followed with commented-out text I'd say go to the root of the issue, make this part of code actually helpful. As it states currently, commented out code does not bring information, the English comments are either to be eliminated or to be significantly shorten because they do not carry enough information. More generally: I think the time which is being spent on this discussion is not being spent for good. If the goal is to improve comments in Blender, the focus must be on their content, not on decor. > This was sent some days ago: https://lists.blender.org/pipermail/bf-committers/2020-October/050688.html Eeeh. Missed that in my inbox somehow. Ok, so you've followed all known practices to get core team involved, without much feedback from the team given =\

In my opinion, we shouldn't even be committing commented-out code.

In the example you give -- I fully agree.

Now, more tricky part.

namespace internal {
...
}  // namespace internal

#ifdef __KERNEL_CPU__
...
#endif  // __KERNEL_CPU__

I find it useful (at least be able) to state what is being "closed". Technically, this is code. And using // namespace internal instead of /* namespace internal */ is just easier and tidier.

Not sure the initial motivation for // was really advocating for "dead code" being committed. If that's the case, my answer to that is that dead code must be removed.

> In my opinion, we shouldn't even be committing commented-out code. In the example you give -- I fully agree. Now, more tricky part. ``` namespace internal { ... } // namespace internal #ifdef __KERNEL_CPU__ ... #endif // __KERNEL_CPU__ ``` I find it useful (at least be able) to state what is being "closed". Technically, this is code. And using `// namespace internal` instead of `/* namespace internal */` is just easier and tidier. Not sure the initial motivation for `//` was really advocating for "dead code" being committed. If that's the case, my answer to that is that dead code must be removed.

Much excitement is going on here now! There is one point I am forgetting to raise about

This can be an exception to the rule, as with any C++ comments that require being inline.

Think how the rule is worded: Use /**/ comments for English text only, unless you are in C++ and unless you're marking arguments as unused . That is how lawyers jobs are being created eh ;)

The official guidelines must be brief, clear, intuitive. Otherwise people will have a big struggle following them.

Much excitement is going on here now! There is one point I am forgetting to raise about > This can be an exception to the rule, as with any C++ comments that require being inline. Think how the rule is worded: Use `/**/` comments for English text only, unless you are in C++ and unless you're marking arguments as unused <here goes all other exceptions>. That is how lawyers jobs are being created eh ;) The official guidelines must be brief, clear, intuitive. Otherwise people will have a big struggle following them.

I don't like the "these are equal to the language, so we give them a different meaning" sentiment. I know @ideasman42 will disagree with me here, but this to me is very similar to the Python quoting style where single quotes indicate "constants" and double quotes indicate "text". I'm more of the "let's use the approach that produces the clearest results", like this:

a = "he said \"Hi\", after \"quotes\" were \"quoted\""
b = 'he said "Hi", after "quotes" were "quoted"'

My point is that, when languages provide some flexibility, it is usually for a good reason (or they couldn't decide on a standardised way, which I don't think applies here). As we've seen above, there are situations where either // or /**/ are clearly better than the other one.

I don't like the "these are equal to the language, so we give them a different meaning" sentiment. I know @ideasman42 will disagree with me here, but this to me is very similar to the Python quoting style where single quotes indicate "constants" and double quotes indicate "text". I'm more of the "let's use the approach that produces the clearest results", like this: ``` a = "he said \"Hi\", after \"quotes\" were \"quoted\"" b = 'he said "Hi", after "quotes" were "quoted"' ``` My point is that, when languages provide some flexibility, it is usually for a good reason (or they couldn't decide on a standardised way, which I don't think applies here). As we've seen above, there are situations where either `//` or `/**/` are clearly better than the other one.
Author
Owner

@dr.sybren the problem with using both is you end up with similar issues to mixing code style. Futher, it's not just a superficial differences, since developers need to configure there editors shortcuts to use comment style most common in the project.

So even if we are not so strict on this, I think it's reasonable to prefer one as part of the code-style.

@dr.sybren the problem with using both is you end up with similar issues to mixing code style. Futher, it's not just a superficial differences, since developers need to configure there editors shortcuts to use comment style most common in the project. So even if we are not so strict on this, I think it's reasonable to prefer one as part of the code-style.
Author
Owner

In #81452#1035476, @Sergey wrote:
C++-style comments:

  • ✗ Looks the same for commented code and text
  • ✓ Brief
  • ✓ Intrinsically free from trade-offs mentioned for C-style, do not cause visual noise in diffs

This isn't true, if we convert C++ comments to the doxygen style comments we use everywhere - it adds a lot of diff noise.

> In #81452#1035476, @Sergey wrote: > C++-style comments: > - ✗ Looks the same for commented code and text > - ✓ Brief > - ✓ Intrinsically free from trade-offs mentioned for C-style, do not cause visual noise in diffs This isn't true, if we convert C++ comments to the doxygen style comments we use everywhere - it adds a lot of diff noise.
Author
Owner

In #81452#1035496, @Sergey wrote:
Think how the rule is worded: Use /**/ comments for English text only, unless you are in C++ and unless you're marking arguments as unused . That is how lawyers jobs are being created eh ;)

The official guidelines must be brief, clear, intuitive. Otherwise people will have a big struggle following them.

Indeed, attempting to put this in a single statement doesn't work well, instead the basic rules can be stated. Any exceptions listed afterwards.

> In #81452#1035496, @Sergey wrote: > Think how the rule is worded: Use `/**/` comments for English text only, unless you are in C++ and unless you're marking arguments as unused <here goes all other exceptions>. That is how lawyers jobs are being created eh ;) > > The official guidelines must be brief, clear, intuitive. Otherwise people will have a big struggle following them. Indeed, attempting to put this in a single statement doesn't work well, instead the basic rules can be stated. Any exceptions listed afterwards.

In #81452#1035444, @dr.sybren wrote:
In my opinion, we shouldn't even be committing commented-out code. It tends to rot and just sit there, and when the time comes to uncomment it, it may actually be doing the wrong thing. Just remove the code.

Nobody is responding to this. Does that mean everybody agrees we shouldn't be choosing our commenting style based on how easy it is to comment out code?

> In #81452#1035444, @dr.sybren wrote: > In my opinion, we shouldn't even be committing commented-out code. It tends to rot and just sit there, and when the time comes to uncomment it, it may actually be doing the wrong thing. Just remove the code. Nobody is responding to this. Does that mean everybody agrees we shouldn't be choosing our commenting style based on how easy it is to comment out code?
Author
Owner

In #81452#1035538, @dr.sybren wrote:

In #81452#1035444, @dr.sybren wrote:
In my opinion, we shouldn't even be committing commented-out code. It tends to rot and just sit there, and when the time comes to uncomment it, it may actually be doing the wrong thing. Just remove the code.

Nobody is responding to this. Does that mean everybody agrees we shouldn't be choosing our commenting style based on how easy it is to comment out code?

We have over 1.5k lines of commented code just under source/ (using "\/\/\s*[^\n]+;").
So even if it's agreed that we shouldn't - in general. This is a fairly large task to remove it, or convert it to #if 0 where keeping it makes sense.

Also, I'm looking to document what we're already doing, not to define something too new and different, which is a bigger task.

Personally I find the existing conventions for comment-style used in most of the code quite reasonable. If there are areas of code that have too much commented, code... OK, that could be addressed, but I'm not proposing any big changes for this.

> In #81452#1035538, @dr.sybren wrote: >> In #81452#1035444, @dr.sybren wrote: >> In my opinion, we shouldn't even be committing commented-out code. It tends to rot and just sit there, and when the time comes to uncomment it, it may actually be doing the wrong thing. Just remove the code. > > Nobody is responding to this. Does that mean everybody agrees we shouldn't be choosing our commenting style based on how easy it is to comment out code? We have over 1.5k lines of commented code just under `source/` (using `"\/\/\s*[^\n]+;"`). So even if it's agreed that we shouldn't - in general. This is a fairly large task to remove it, or convert it to `#if 0` where keeping it makes sense. Also, I'm looking to document what we're already doing, not to define something too new and different, which is a bigger task. Personally I find the existing conventions for comment-style used in most of the code quite reasonable. If there are areas of code that have too much commented, code... OK, that could be addressed, but I'm not proposing any big changes for this.

For commenting out code, I would say it's discouraged, and not even bother to define a convention for something that we shouldn't be doing.

C style has been the convention for a long time and most code uses it (including after #endif). There's pros and cons,, but I don't think it's a good use of time to change conventions here.

For commenting out code, I would say it's discouraged, and not even bother to define a convention for something that we shouldn't be doing. C style has been the convention for a long time and most code uses it (including after `#endif`). There's pros and cons,, but I don't think it's a good use of time to change conventions here.

There is a convention, there is a guide which is worded in terms of "preferred". If we don't change the wording/meaning/convention I would like not to see project-wide cleanups (which switches from acceptable to preferred state).

There is a convention, there is a guide which is worded in terms of "preferred". If we don't change the wording/meaning/convention I would like not to see project-wide cleanups (which switches from acceptable to preferred state).
Author
Owner

If everyone can prefer their own style, there is not so much point in settling on a convention - it's back to "use the style of whoever last worked in that area".

Getting away from this is one of the reason I made the effort to work on this proposal, it's annoying and easily avoidable.
We can pick some conventions - use them everywhere and move on (similar to what we did with clang-format).

I thought this wasn't going to be too controversial by using something that's already being done in nearly all of Blender's code.

If everyone can prefer their own style, there is not so much point in settling on a convention - it's back to *"use the style of whoever last worked in that area"*. Getting away from this is one of the reason I made the effort to work on this proposal, it's annoying and easily avoidable. We can pick some conventions - use them everywhere and move on (similar to what we did with `clang-format`). I thought this wasn't going to be too controversial by using something that's already being done in nearly all of Blender's code.
Member

@ideasman42 - Off topic, but your hope for "running a spell checker on comments" was my motivation for correcting the types of spelling/grammar problems that are typically not caught by spellcheckers, like accidentally using the wrong word:

These changes, plus spellchecking, should put us in a nice shape.

@ideasman42 - Off topic, but your hope for "running a spell checker on comments" was my motivation for correcting the types of spelling/grammar problems that are typically **not caught by spellcheckers**, like accidentally using the wrong word: - [[D9243](https://archive.blender.org/developer/D9243) - Loose Versus Lose ](https://developer.blender.org/D9243) - [[D9245](https://archive.blender.org/developer/D9245) - Apart Versus A Part ](https://developer.blender.org/D9245) - [[D9246](https://archive.blender.org/developer/D9246) - Then Versus Than ](https://developer.blender.org/D9246) - [[D9250](https://archive.blender.org/developer/D9250) - It's Versus Its ](https://developer.blender.org/D9250) - [[D9248](https://archive.blender.org/developer/D9248) - Miscellaneous ](https://developer.blender.org/D9248) These changes, plus spellchecking, should put us in a nice shape.

In #81452#1035689, @ideasman42 wrote:
I thought this wasn't going to be too controversial by using something that's already being done in nearly all of Blender's code.

Sorry for sounding sour here, but I don't think Blender's current state should be used as example of how things should be done. When I started digging into Blender's code (granted, that was a while ago) it was incredibly under-documented. There are still large chunks that have little to no documentation, are overly complex, and use abbreviated-beyond-recognision variable names. I think we're going the right way with Blender, and that its quality has improved a lot recently. My point is that it's fine to hear various ideas from people on how they think the situation could be improved, and then make a decision on how we'll put things into guidelines. However, I don't think it's fair to give too much weight to Blender's current state of code to form decisions, as that basically translates to "we keep doing what the old farts have always been doing".

Of course everybody prefers their own style. This is the time and place to voice that, right? To present the motivations for those preferences, and to come to a balanced conclusion we can apply to all of Blender? I thought that that was what we were doing, anyway.

> In #81452#1035689, @ideasman42 wrote: > I thought this wasn't going to be too controversial by using something that's already being done in nearly all of Blender's code. Sorry for sounding sour here, but I don't think Blender's current state should be used as example of how things should be done. When I started digging into Blender's code (granted, that was a while ago) it was incredibly under-documented. There are still large chunks that have little to no documentation, are overly complex, and use abbreviated-beyond-recognision variable names. I think we're going the right way with Blender, and that its quality has improved a lot recently. My point is that it's fine to hear various ideas from people on how they think the situation could be improved, and then make a decision on how we'll put things into guidelines. However, I don't think it's fair to give too much weight to Blender's current state of code to form decisions, as that basically translates to "we keep doing what the old farts have always been doing". Of course everybody prefers their own style. This is the time and place to voice that, right? To present the motivations for those preferences, and to come to a balanced conclusion we can apply to all of Blender? I thought that that was what we were doing, anyway.
Author
Owner

In #81452#1039161, @dr.sybren wrote:

In #81452#1035689, @ideasman42 wrote:
I thought this wasn't going to be too controversial by using something that's already being done in nearly all of Blender's code.

Sorry for sounding sour here, but I don't think Blender's current state should be used as example of how things should be done. When I started digging into Blender's code (granted, that was a while ago) it was incredibly under-documented. There are still large chunks that have little to no documentation, are overly complex, and use abbreviated-beyond-recognision variable names.

While it's of course valid to question old conventions. This doesn't make them bad by definition. Issues with comment quality are quite separate from syntax used for commenting.

I think we're going the right way with Blender, and that its quality has improved a lot recently. My point is that it's fine to hear various ideas from people on how they think the situation could be improved, and then make a decision on how we'll put things into guidelines. However, I don't think it's fair to give too much weight to Blender's current state of code to form decisions, as that basically translates to "we keep doing what the old farts have always been doing".

Point taken, however the onus is on the person proposing large changes to prove this is an overall benefit.

  • If automated, this is quite disruptive, similar to running clang-format again.
  • If done manually, this will happen slowly over a long period of time, leaving the state of comments mixed, also, with the down side that manually converting the comments takes developer time. Also, this is still disruptive (breaking patches, causing merge conflicts etc... just spread out over months/years).
> In #81452#1039161, @dr.sybren wrote: >> In #81452#1035689, @ideasman42 wrote: >> I thought this wasn't going to be too controversial by using something that's already being done in nearly all of Blender's code. > Sorry for sounding sour here, but I don't think Blender's current state should be used as example of how things should be done. When I started digging into Blender's code (granted, that was a while ago) it was incredibly under-documented. There are still large chunks that have little to no documentation, are overly complex, and use abbreviated-beyond-recognision variable names. While it's of course valid to question old conventions. This doesn't make them bad by definition. Issues with comment quality are quite separate from syntax used for commenting. > I think we're going the right way with Blender, and that its quality has improved a lot recently. My point is that it's fine to hear various ideas from people on how they think the situation could be improved, and then make a decision on how we'll put things into guidelines. However, I don't think it's fair to give too much weight to Blender's current state of code to form decisions, as that basically translates to "we keep doing what the old farts have always been doing". Point taken, however the onus is on the person proposing large changes to prove this is an overall benefit. - If automated, this is quite disruptive, similar to running `clang-format` again. - If done manually, this will happen slowly over a long period of time, leaving the state of comments mixed, also, with the down side that manually converting the comments takes developer time. Also, this is still disruptive (breaking patches, causing merge conflicts etc... just spread out over months/years).

Can we just treat this as a task to document existing guidelines, which is already a net improvement?

It's fine to have discussion beyond that, but I don't think it's @ideasman42's responsibility to do anything with that. There's no need for something like this to drag on for 3 weeks.

Can we just treat this as a task to document existing guidelines, which is already a net improvement? It's fine to have discussion beyond that, but I don't think it's @ideasman42's responsibility to do anything with that. There's no need for something like this to drag on for 3 weeks.

In #81452#1041511, @ideasman42 wrote:
While it's of course valid to question old conventions. This doesn't make them bad by definition. Issues with comment quality are quite separate from syntax used for commenting.

True.

Point taken, however the onus is on the person proposing large changes to prove this is an overall benefit.

  • If automated, this is quite disruptive, similar to running clang-format again.

Personally I haven't experienced much disruption from Clang-Format, and I'm very happy that we're actively using it now. Especially given that D9234: git blame: add file to help ignore cleanup commits is a possibility, I don't see a big issue here.

In #81452#1041632, @brecht wrote:
Can we just treat this as a task to document existing guidelines, which is already a net improvement?

I would argue that something cannot be an "existing guideline" if it wasn't documented. Without documenting it as a guideline, it's just something that certain developers happen to have done.

The Alembic & USD code doesn't adhere to the "// only for commented-out code", so turning that into a guideline will also cause code changes.

What I'm suggesting won't cause any big changes. I'm just not a fan of limiting the use of * to commenting out code, when pretty much any clean coding book/guide/article I've read says "don't commit commented-out code". I think that for some debug-printing calls we can make an exception, because it's really only for debugging things. However, there are plenty of commented-out (of #ifdefed) blocks of code that have lost their value. On the other side, as @Sergey and @LazyDodo described, there are legitimate advantages of * for regular comments.

It's fine to have discussion beyond that, but I don't think it's @ideasman42's responsibility to do anything with that. There's no need for something like this to drag on for 3 weeks.

It's fine by me to just leave out this bit from the code comment style guide, and discuss it in a separate task. All the other suggested changes are improvements IMO.

> In #81452#1041511, @ideasman42 wrote: > While it's of course valid to question old conventions. This doesn't make them bad by definition. Issues with comment quality are quite separate from syntax used for commenting. True. > Point taken, however the onus is on the person proposing large changes to prove this is an overall benefit. > > - If automated, this is quite disruptive, similar to running `clang-format` again. Personally I haven't experienced much disruption from Clang-Format, and I'm very happy that we're actively using it now. Especially given that [D9234: git blame: add file to help ignore cleanup commits](https://archive.blender.org/developer/D9234) is a possibility, I don't see a big issue here. > In #81452#1041632, @brecht wrote: > Can we just treat this as a task to document existing guidelines, which is already a net improvement? I would argue that something cannot be an "existing guideline" if it wasn't documented. Without documenting it as a guideline, it's just something that certain developers happen to have done. The Alembic & USD code doesn't adhere to the "`//` only for commented-out code", so turning that into a guideline will also cause code changes. What I'm suggesting won't cause any big changes. I'm just not a fan of limiting the use of `*` to commenting out code, when pretty much any clean coding book/guide/article I've read says "don't commit commented-out code". I think that for some debug-printing calls we can make an exception, because it's really only for debugging things. However, there are plenty of commented-out (of `#ifdef`ed) blocks of code that have lost their value. On the other side, as @Sergey and @LazyDodo described, there are legitimate advantages of `*` for regular comments. > It's fine to have discussion beyond that, but I don't think it's @ideasman42's responsibility to do anything with that. There's no need for something like this to drag on for 3 weeks. It's fine by me to just leave out this bit from the code comment style guide, and discuss it in a separate task. All the other suggested changes are improvements IMO.

Think some of the points which I've mentioned on blender.chat did not end up here.

There are topics which are adding a clear value, which we agree on. Those should be moved on to wiki. And, more importantly, followed in the new code, patches review and so on. To have such things better organized it helps keeping an agenda of topics, with status: is it discussed, is it agreed on, is further discussion needed.

For the responsibility I think it is responsibility of an author of a proposal to keep track of it, making sure it is moving forward. I do not think it will be sustainable state if developers started to write proposals they can not work on, can not followup on, can not keep track of.

Think some of the points which I've mentioned on blender.chat did not end up here. There are topics which are adding a clear value, which we agree on. Those should be moved on to wiki. And, more importantly, followed in the new code, patches review and so on. To have such things better organized it helps keeping an agenda of topics, with status: is it discussed, is it agreed on, is further discussion needed. For the responsibility I think it is responsibility of an author of a proposal to keep track of it, making sure it is moving forward. I do not think it will be sustainable state if developers started to write proposals they can not work on, can not followup on, can not keep track of.
Author
Owner

In #81452#1041668, @dr.sybren wrote:

Point taken, however the onus is on the person proposing large changes to prove this is an overall benefit.

  • If automated, this is quite disruptive, similar to running clang-format again.

Personally I haven't experienced much disruption from Clang-Format, and I'm very happy that we're actively using it now. Especially given that D9234: git blame: add file to help ignore cleanup commits is a possibility, I don't see a big issue here.

There is still the issue of causing conflicts with branches & patches. Causing us to spend time manually updating patches.

In #81452#1041632, @brecht wrote:
Can we just treat this as a task to document existing guidelines, which is already a net improvement?

I would argue that something cannot be an "existing guideline" if it wasn't documented. Without documenting it as a guideline, it's just something that certain developers happen to have done.

No, this is documented.

The existing guidelines state It is preferred to use C-style comments in C++ code too.

While it's only "preferred", it's still an existing guideline which has been followed for the most part.

The Alembic & USD code doesn't adhere to the "// only for commented-out code", so turning that into a guideline will also cause code changes.

This is a small fraction of Blender's overall code-base. If we settle on a convention - it's going to cause some change. What I'm proposing will cause the least amount of change since it's mainly re-reinforcing the current guide-lines.

What I'm suggesting won't cause any big changes.

In that case are you're proposing to let everyone do what they want? (removing the preference for one comment style over another).

I'm just not a fan of limiting the use of // to commenting out code, when pretty much any clean coding book/guide/article I've read says "don't commit commented-out code". I think that for some debug-printing calls we can make an exception, because it's really only for debugging things.

Even if the amount of commented code is significantly reduced to whatever is deemed acceptable, we will still likely have a fair amount. Even if we're only counting commented printf's and flags used in DNA which are still set in older files.

However, there are plenty of commented-out (of #ifdefed) blocks of code that have lost their value. On the other side, as @Sergey and @LazyDodo described, there are legitimate advantages of // for regular comments.

There are legitimate disadvantages too, moving a comment block to doxygen syntax will cause much noisier diff's (since all lines need to be edited).

I'd argue this is basically a personal preference as there are pros/cons with both. If we're going to be concerned by single-character space use, we could also argue for camelCase instead of snake_case.

It's fine to have discussion beyond that, but I don't think it's @ideasman42's responsibility to do anything with that. There's no need for something like this to drag on for 3 weeks.

It's fine by me to just leave out this bit from the code comment style guide, and discuss it in a separate task. All the other suggested changes are improvements IMO.

Leaving it out just prolongs the discussion, I'm not sure this needs a lot more time spent on it since I think everyone who feels strongly about this has made their point of view known here.

In #81452#1041680, @Sergey wrote:
For the responsibility I think it is responsibility of an author of a proposal to keep track of it, making sure it is moving forward. I do not think it will be sustainable state if developers started to write proposals they can not work on, can not followup on, can not keep track of.

Agree that the developer making the proposal needs to be responsive & keep track of the proposal.
Larger changes (which require global changes to the code-base) being proposed by others should have their own proposal.

For example - the author of a proposal to improve docs on our API naming should not have to spend significant time defending the use of snake_case instead of camelCase for our function names.

In general it's not practical to have to re-open long discussions about the status-quo as part of proposals to document it.

> In #81452#1041668, @dr.sybren wrote: >> Point taken, however the onus is on the person proposing large changes to prove this is an overall benefit. >> >> - If automated, this is quite disruptive, similar to running `clang-format` again. > > Personally I haven't experienced much disruption from Clang-Format, and I'm very happy that we're actively using it now. Especially given that [D9234: git blame: add file to help ignore cleanup commits](https://archive.blender.org/developer/D9234) is a possibility, I don't see a big issue here. There is still the issue of causing conflicts with branches & patches. Causing us to spend time manually updating patches. >> In #81452#1041632, @brecht wrote: >> Can we just treat this as a task to document existing guidelines, which is already a net improvement? > > I would argue that something cannot be an "existing guideline" if it wasn't documented. Without documenting it as a guideline, it's just something that certain developers happen to have done. No, this is documented. The existing guidelines state `It is preferred to use C-style comments in C++ code too.` While it's only "preferred", it's still an existing guideline which has been followed for the most part. > The Alembic & USD code doesn't adhere to the "`//` only for commented-out code", so turning that into a guideline will also cause code changes. This is a small fraction of Blender's overall code-base. If we settle on a convention - it's going to cause _some_ change. What I'm proposing will cause the least amount of change since it's mainly re-reinforcing the current guide-lines. > What I'm suggesting won't cause any big changes. In that case are you're proposing to let everyone do what they want? (removing the preference for one comment style over another). > I'm just not a fan of limiting the use of `//` to commenting out code, when pretty much any clean coding book/guide/article I've read says "don't commit commented-out code". I think that for some debug-printing calls we can make an exception, because it's really only for debugging things. Even if the amount of commented code is significantly reduced to whatever is deemed acceptable, we will still likely have a fair amount. Even if we're only counting commented printf's and flags used in DNA which are still set in older files. > However, there are plenty of commented-out (of `#ifdef`ed) blocks of code that have lost their value. On the other side, as @Sergey and @LazyDodo described, there are legitimate advantages of `//` for regular comments. There are legitimate disadvantages too, moving a comment block to doxygen syntax will cause much noisier diff's (since all lines need to be edited). I'd argue this is basically a personal preference as there are pros/cons with both. If we're going to be concerned by single-character space use, we could also argue for `camelCase` instead of `snake_case`. >> It's fine to have discussion beyond that, but I don't think it's @ideasman42's responsibility to do anything with that. There's no need for something like this to drag on for 3 weeks. > > It's fine by me to just leave out this bit from the code comment style guide, and discuss it in a separate task. All the other suggested changes are improvements IMO. Leaving it out just prolongs the discussion, I'm not sure this needs a lot more time spent on it since I think everyone who feels strongly about this has made their point of view known here. > In #81452#1041680, @Sergey wrote: > For the responsibility I think it is responsibility of an author of a proposal to keep track of it, making sure it is moving forward. I do not think it will be sustainable state if developers started to write proposals they can not work on, can not followup on, can not keep track of. Agree that the developer making the proposal needs to be responsive & keep track of the proposal. Larger changes (which require global changes to the code-base) being proposed by others should have their own proposal. For example - the author of a proposal to improve docs on our API naming should not have to spend significant time defending the use of `snake_case` instead of `camelCase` for our function names. In general it's not practical to have to re-open long discussions about the status-quo as part of proposals to document it.

Added subscriber: @ZedDB

Added subscriber: @ZedDB

I'll just throw in my two cents:

I know this will probably not change because we have had this convention for a while but I think:

/* */ style comments only belong to bigger comments where you have some neat formatting or multiple paragraphs.

For everything else I prefer to use // comments.
This is because they are easier to work with and faster to type.

Especially since we have the "Comments must end with period" rule. Because of the */ we essentially have "full stop" two times.
It's like having two write \r\n to do a proper newline instead of just \n ;)

I'll just throw in my two cents: I know this will probably not change because we have had this convention for a while but I think: `/* */` style comments only belong to bigger comments where you have some neat formatting or multiple paragraphs. For everything else I prefer to use `//` comments. This is because they are easier to work with and faster to type. Especially since we have the "Comments must end with period" rule. Because of the `*/` we essentially have "full stop" two times. It's like having two write `\r\n` to do a proper newline instead of just `\n` ;)
Author
Owner

In #81452#1046089, @ZedDB wrote:
I'll just throw in my two cents:

I know this will probably not change because we have had this convention for a while but I think:

/* */ style comments only belong to bigger comments where you have some neat formatting or multiple paragraphs.

For everything else I prefer to use // comments.

I get where you're coming from, my main concern with this is - in practice it means extending existing comments would be tedious - or developers would forget to change the comment style & we'd end up with a mix of both... having to make clean-up commits to enforce the rule.

This is because they are easier to work with and faster to type.

Not sure about this one, don't most developers have shortcuts setup for commenting?

Especially since we have the "Comments must end with period" rule. Because of the */ we essentially have "full stop" two times.
It's like having two write \r\n to do a proper newline instead of just \n ;)

You're not wrong but I don't have a strong opinion on this one, seems like a minor difference... you might end with ! or ? too :)

> In #81452#1046089, @ZedDB wrote: > I'll just throw in my two cents: > > I know this will probably not change because we have had this convention for a while but I think: > > `/* */` style comments only belong to bigger comments where you have some neat formatting or multiple paragraphs. > > For everything else I prefer to use `//` comments. I get where you're coming from, my main concern with this is - in practice it means extending existing comments would be tedious - or developers would forget to change the comment style & we'd end up with a mix of both... having to make clean-up commits to enforce the rule. > This is because they are easier to work with and faster to type. Not sure about this one, don't most developers have shortcuts setup for commenting? > Especially since we have the "Comments must end with period" rule. Because of the `*/` we essentially have "full stop" two times. > It's like having two write `\r\n` to do a proper newline instead of just `\n` ;) You're not wrong but I don't have a strong opinion on this one, seems like a minor difference... you might end with `!` or `?` too :)

In #81452#1046221, @ideasman42 wrote:

I get where you're coming from, my main concern with this is - in practice it means extending existing comments would be tedious - or developers would forget to change the comment style & we'd end up with a mix of both... having to make clean-up commits to enforce the rule.

I've already had to fix old comments that didn't follow our current guidelines in cleanup commits, so we already have issues like this.

If we wanted to, then we could perhaps have some mindless task (like the clang-tidy task) where we go over each file during a "code quality day" and check it off when it is deemed to follow our current comment standard.

But the question is if these cleanup tasks would be worth it. :P

Not sure about this one, don't most developers have shortcuts setup for commenting?

Sure, I already have it setup so it automatically adds * on newlines as well.
But I'm more inclined to adopt a comment style that we don't have to workaround with editor macros .

You're not wrong but I don't have a strong opinion on this one, seems like a minor difference... you might end with ! or ? too :)

My point is that with // style comments, this is not an issue as you don't have a "end of comment" denominator.

> In #81452#1046221, @ideasman42 wrote: > > I get where you're coming from, my main concern with this is - in practice it means extending existing comments would be tedious - or developers would forget to change the comment style & we'd end up with a mix of both... having to make clean-up commits to enforce the rule. > I've already had to fix old comments that didn't follow our current guidelines in cleanup commits, so we already have issues like this. If we wanted to, then we could perhaps have some mindless task (like the clang-tidy task) where we go over each file during a "code quality day" and check it off when it is deemed to follow our current comment standard. But the question is if these cleanup tasks would be worth it. :P > > Not sure about this one, don't most developers have shortcuts setup for commenting? > Sure, I already have it setup so it automatically adds `*` on newlines as well. But I'm more inclined to adopt a comment style that we don't have to workaround with editor macros . > > You're not wrong but I don't have a strong opinion on this one, seems like a minor difference... you might end with `!` or `?` too :) My point is that with `//` style comments, this is not an issue as you don't have a "end of comment" denominator.
Author
Owner

In #81452#1046453, @ZedDB wrote:

In #81452#1046221, @ideasman42 wrote:

I get where you're coming from, my main concern with this is - in practice it means extending existing comments would be tedious - or developers would forget to change the comment style & we'd end up with a mix of both... having to make clean-up commits to enforce the rule.

I've already had to fix old comments that didn't follow our current guidelines in cleanup commits, so we already have issues like this.

Even so, should pick a convention that minimizes accidents and the need for them to be cleaned up.

If we wanted to, then we could perhaps have some mindless task (like the clang-tidy task) where we go over each file during a "code quality day" and check it off when it is deemed to follow our current comment standard.

But the question is if these cleanup tasks would be worth it. :P

Ideally we can pick something straightforward, then we can follow it without having to clean up after ourselves.

Not sure about this one, don't most developers have shortcuts setup for commenting?

Sure, I already have it setup so it automatically adds * on newlines as well.
But I'm more inclined to adopt a comment style that we don't have to workaround with editor macros .

Right, but this change moves in the direction of changing Blender's comment style entirely, not something that would be some incremental changes.

I'd rather that be handled in a separate proposal, since it's not the purpose of this proposal to justify Blender's comment current comment style compared with various personal preferences.
When those preferences would end up changing many thousands of lines of code.

You're not wrong but I don't have a strong opinion on this one, seems like a minor difference... you might end with ! or ? too :)

My point is that with // style comments, this is not an issue as you don't have a "end of comment" denominator.

> In #81452#1046453, @ZedDB wrote: >> In #81452#1046221, @ideasman42 wrote: >> >> I get where you're coming from, my main concern with this is - in practice it means extending existing comments would be tedious - or developers would forget to change the comment style & we'd end up with a mix of both... having to make clean-up commits to enforce the rule. >> > I've already had to fix old comments that didn't follow our current guidelines in cleanup commits, so we already have issues like this. Even so, should pick a convention that minimizes accidents and the need for them to be cleaned up. > If we wanted to, then we could perhaps have some mindless task (like the clang-tidy task) where we go over each file during a "code quality day" and check it off when it is deemed to follow our current comment standard. > > But the question is if these cleanup tasks would be worth it. :P Ideally we can pick something straightforward, then we can follow it without having to clean up after ourselves. >> Not sure about this one, don't most developers have shortcuts setup for commenting? >> > > Sure, I already have it setup so it automatically adds `*` on newlines as well. > But I'm more inclined to adopt a comment style that we don't have to workaround with editor macros . Right, but this change moves in the direction of changing Blender's comment style entirely, not something that would be some incremental changes. I'd rather that be handled in a separate proposal, since it's not the purpose of this proposal to justify Blender's comment current comment style compared with various personal preferences. When those preferences would end up changing many thousands of lines of code. >> You're not wrong but I don't have a strong opinion on this one, seems like a minor difference... you might end with `!` or `?` too :) > > My point is that with `//` style comments, this is not an issue as you don't have a "end of comment" denominator.

@ideasman42 , seems that the only debate is about /**/ vs. // vs #if 0. The rest I'd say: +1, please move to the Wiki ;)

For that one remaining topic seems it is all clear that there are cons and pros in all the camps. Don't think it worth re-iterating on those cons/pros. So, have a vote between admins and carry on with lives?

P.S. Something which is not really up to this discussion, but something what we (I?) stumbled upon here. The definition of "preferred" used in the guide. Seems people have different interpretation of it, so how about avoiding this word for the official "The Code Style" wiki page? For example, phrase n a way "Use comment style.". With the "preferred" used in the guide feels like "you can use an alternative, until cleanup team comes by".

@ideasman42 , seems that the only debate is about `/**/` vs. `//` vs `#if 0`. The rest I'd say: +1, please move to the Wiki ;) For that one remaining topic seems it is all clear that there are cons and pros in all the camps. Don't think it worth re-iterating on those cons/pros. So, have a vote between admins and carry on with lives? P.S. Something which is not really up to this discussion, but something what we (I?) stumbled upon here. The definition of "preferred" used in the guide. Seems people have different interpretation of it, so how about avoiding this word for the official "The Code Style" wiki page? For example, phrase n a way "Use <FOO> comment style.". With the "preferred" used in the guide feels like "you can use an alternative, until cleanup team comes by".
Member

While we are doing things in this area it be great to resolve the ambiguity between

https://wiki.blender.org/wiki/Style_Guide/C_Cpp
https://wiki.blender.org/wiki/User:Sergey/CodeStyle

When you google blender codestyle they are the two first results, having only one would be.... uhmm.. preferred ? :)

While we are doing things in this area it be great to resolve the ambiguity between https://wiki.blender.org/wiki/Style_Guide/C_Cpp https://wiki.blender.org/wiki/User:Sergey/CodeStyle When you google `blender codestyle` they are the two first results, having only one would be.... uhmm.. preferred ? :)

Eeek, hows the initial pass of the style guide even made it to the google. Don't know if it worth having that initial version. If not, can someone please setup a redirect (I really don't really know how do so)?

Eeek, hows the initial pass of the style guide even made it to the google. Don't know if it worth having that initial version. If not, can someone please setup a redirect (I really don't really know how do so)?

I've added the redirect now.

I've added the redirect now.

Don't have opinion on the C/C++ comment styles topic.

Rest of the proposal is fine by me.

Don't have opinion on the C/C++ comment styles topic. Rest of the proposal is fine by me.

In #81452#1110686, @Sergey wrote:
For that one remaining topic seems it is all clear that there are cons and pros in all the camps. Don't think it worth re-iterating on those cons/pros. So, have a vote between admins and carry on with lives?

👍 I can live with any decision the admins make in this regard. In the end I think I prefer clarity of the decision over any one particular choice.

> In #81452#1110686, @Sergey wrote: > For that one remaining topic seems it is all clear that there are cons and pros in all the camps. Don't think it worth re-iterating on those cons/pros. So, have a vote between admins and carry on with lives? :+1: I can live with any decision the admins make in this regard. In the end I think I prefer clarity of the decision over any one particular choice.

C-style comments should be used in C++ code for English text comments. // should be used for disabling source-code.

Suggest rephrasing this in the following way (or something with similar meaning):

C-style comments should be used in C++ code.

Adding dead code is discouraged. In some cases, however, having unused code is useful (gives more semantic meaning, provides reference implementation, ...). It is fine having unused coed in this cases. Use `//` for a single-line code, and `#if 0` for multi-line code. And always explain what the unused code is about.
> C-style comments should be used in C++ code for English text comments. // should be used for disabling source-code. Suggest rephrasing this in the following way (or something with similar meaning): ``` C-style comments should be used in C++ code. Adding dead code is discouraged. In some cases, however, having unused code is useful (gives more semantic meaning, provides reference implementation, ...). It is fine having unused coed in this cases. Use `//` for a single-line code, and `#if 0` for multi-line code. And always explain what the unused code is about. ```
Author
Owner

@Sergey that better reflects how comments are used, updated doc.

@Sergey that better reflects how comments are used, updated doc.
Author
Owner

Changed status from 'Confirmed' to: 'Resolved'

Changed status from 'Confirmed' to: 'Resolved'
Campbell Barton self-assigned this 2021-07-20 04:07:31 +02:00
Author
Owner

From discussion with other admins, we all agreed to go ahead with this proposal. https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments has been updated.

Other suggestions can be made as separate proposals.

From discussion with other admins, we all agreed to go ahead with this proposal. https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments has been updated. Other suggestions can be made as separate proposals.

This issue was referenced by 2bc0e8d304

This issue was referenced by 2bc0e8d3049ec1b7c775717ec7aff6e9ab1c7726
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
13 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#81452
No description provided.