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.

In T81452#1035689, @Campbell Barton (campbellbarton) 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 T81452#1039161, @Sybren A. Stüvel (sybren) wrote:

In T81452#1035689, @Campbell Barton (campbellbarton) 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 @Campbell Barton (campbellbarton)'s responsibility to do anything with that. There's no need for something like this to drag on for 3 weeks.

In T81452#1041511, @Campbell Barton (campbellbarton) 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 T81452#1041632, @Brecht Van Lommel (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 Sharybin (sergey) and @Ray molenkamp (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 @Campbell Barton (campbellbarton)'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.

In T81452#1041668, @Sybren A. Stüvel (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 T81452#1041632, @Brecht Van Lommel (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 Sharybin (sergey) and @Ray molenkamp (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 @Campbell Barton (campbellbarton)'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 T81452#1041680, @Sergey Sharybin (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.

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

In T81452#1046089, @Sebastian Parborg (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 T81452#1046221, @Campbell Barton (campbellbarton) 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 T81452#1046453, @Sebastian Parborg (zeddb) wrote:

In T81452#1046221, @Campbell Barton (campbellbarton) 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.

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

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

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.

In T81452#1110686, @Sergey Sharybin (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.

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.

@Sergey Sharybin (sergey) that better reflects how comments are used, updated doc.