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?

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.

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

@Campbell Barton (campbellbarton) List to Last?

In T81452#1028337, @Sybren A. Stüvel (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 T81452#1028339, @Julian Eisel (Severin) 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 T81452#1028929, @Campbell Barton (campbellbarton) wrote:

In T81452#1028339, @Julian Eisel (Severin) 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.

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.

In T81452#1029052, @Brecht Van Lommel (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 T81452#1029027, @Sybren A. Stüvel (sybren) wrote:

In T81452#1028929, @Campbell Barton (campbellbarton) wrote:

In T81452#1028339, @Julian Eisel (Severin) 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 T81452#1029052, @Brecht Van Lommel (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.

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.

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.

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.

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

In T81452#1030338, @Harley Acheson (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#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.

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.

• 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 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.

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.

In T81452#1032443, @Sybren A. Stüvel (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 T81452#1032682, @Ray molenkamp (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.

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 "

In T81452#1032909, @Ray molenkamp (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);

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.

In T81452#1035430, @Sergey Sharybin (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 T81452#1032682, @Ray molenkamp (LazyDodo) wrote:

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

I fully agree.

In T81452#1032909, @Ray molenkamp (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 Sharybin (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 =\

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 <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 @Campbell Barton (campbellbarton) 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.

@Sybren A. Stüvel (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.

In T81452#1035476, @Sergey Sharybin (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 T81452#1035496, @Sergey Sharybin (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 T81452#1035444, @Sybren A. Stüvel (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 T81452#1035538, @Sybren A. Stüvel (sybren) wrote:

In T81452#1035444, @Sybren A. Stüvel (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.

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).

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.

@Campbell Barton (campbellbarton) - 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 - Loose Versus Lose
• D9245 - Apart Versus A Part
• D9246 - Then Versus Than
• D9250 - It's Versus Its
• D9248 - Miscellaneous

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