Policy for style guide: doxygen sections #74845

New Issue

Campbell Barton · 2020-03-17T12:55:32+01:00

Campbell Barton commented

2020-03-17 12:55:32 +01:00

Note that this proposal is about using sections:

Small single purpose files need not use sections at all.
Large files which require sections to be navigated could be split into files.

This proposal covers the case when the developer wants to group code into sections, for whatever reason it's not practical to split the file into smaller parts.

While currently doxygen sections are used in Blender's code (over 1900 sections at time of writing), this isn't part of the style guide.

This means there are still places developers define their own add-hoc sections.
While this in it's self isn't so bad, it means the topic is raised when developers are editing code - if doxygen should be used in an area of code or not, as well as each developer having their own personal style.

Pros:

All Blender code uses the same convention for sections.
Developers can use utilities to navigate between sections - which work across all files.
Beginning and sections make it clear when adding code to the bottom of a file that it's part of a section. Something which is often forgotten with header-only add-hoc sections.
It improves readability of generated doxygen. Currently this is quite a small pro - as we don't publish generated doxygen at the moment.

Cons:

More verbose than simple one line comments.
Inconvenient to type.

This proposal is to add doxy sections to the style guide , something like this.

When grouping code into sections doxygen syntax should be used, with an optional over-line for clarity, eg:
/* -------------------------------------------------------------------- */
/** \name Title of Code Section
 * \{ */

...

/** \} */
You may include descriptive text about the section under the title:
/* -------------------------------------------------------------------- */
/** \name Title of Code Section
 *
 * Explain in more detail the purpose of the section.
 * \{ */

...

/** \} */

Screenshot of doxy sections in an editor which syntax highlights doxygen.

Where non-doxygen Sections are acceptable.

Developers may want to use shorter sections for headers which mainly contain declarations.

In this case the following sections are acceptable, proposed wiki text.

For headers that mainly contain declarations, the following non-doxy sections are also acceptable.
/* --------------------------------------------------------------------
 * Name of the section.
 */
Or with a some extra text:
/* --------------------------------------------------------------------
 * Name of the section.
 *
 * Optional description.
 */

Note that this proposal is about using sections: - Small single purpose files need not use sections at all. - Large files which require sections to be navigated could be split into files. This proposal covers the case when the developer wants to group code into sections, for whatever reason it's not practical to split the file into smaller parts. ---- While currently [doxygen sections ](http://www.doxygen.nl/manual/grouping.html) are used in Blender's code (over 1900 sections at time of writing), this isn't part of the style guide. This means there are still places developers define their own add-hoc sections. While this in it's self isn't so bad, it means the topic is raised when developers are editing code - if doxygen should be used in an area of code or not, as well as each developer having their own personal style. Pros: - All Blender code uses the same convention for sections. - Developers can use utilities to navigate between sections - which work across all files. - Beginning and sections make it clear when adding code to the bottom of a file that it's part of a section. *Something which is often forgotten with header-only add-hoc sections.* - It improves readability of generated doxygen. *Currently this is quite a small pro - as we don't publish generated doxygen at the moment.* Cons: - More verbose than simple one line comments. - Inconvenient to type. This proposal is to add doxy sections to the [style guide ](https://wiki.blender.org/wiki/Source/Code_Style), something like this. ---- > When grouping code into sections doxygen syntax should be used, with an optional over-line for clarity, eg: > ``` > /* -------------------------------------------------------------------- */ > /** \name Title of Code Section > * \{ */ > > ... > > /** \} */ > ``` > > You may include descriptive text about the section under the title: > > ``` > /* -------------------------------------------------------------------- */ > /** \name Title of Code Section > * > * Explain in more detail the purpose of the section. > * \{ */ > > ... > > /** \} */ > ``` Screenshot of doxy sections in an editor which syntax highlights doxygen. ![doxy_sections.png](https://archive.blender.org/developer/F8412050/doxy_sections.png) ----- ## Where non-doxygen Sections are acceptable. Developers may want to use shorter sections for headers which mainly contain declarations. In this case the following sections are acceptable, proposed wiki text. > For headers that mainly contain declarations, the following non-doxy sections are also acceptable. > > ``` > /* -------------------------------------------------------------------- > * Name of the section. > */ > ``` > > Or with a some extra text: > > ``` > /* -------------------------------------------------------------------- > * Name of the section. > * > * Optional description. > */ > ```

Campbell Barton commented

2020-03-17 12:55:32 +01:00

Added subscribers: @ideasman42, @rjg

Campbell Barton commented

2020-03-17 12:56:41 +01:00

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

Campbell Barton changed title from ~~Policy for style guids: doxygen sections~~ to Policy for style guide: doxygen sections

2020-03-17 13:24:41 +01:00

Sergey Sharybin commented

2020-03-17 13:48:49 +01:00

Added subscriber: @Sergey

Sergey Sharybin commented

2020-03-17 13:48:49 +01:00

While i agree having some standard way to define sections is useful, the doxygen have big downsides to me.

All Blender code uses the same convention for sections.

You can have consistency for section with whatever-style.

Developers can use utilities to navigate between sections - which work across all files.

There is nothing really ready-to-go for this in any IDE. For the cusomization scripts you can train them to use whatever-regex.

Beginning and sections make it clear when adding code to the bottom of a file that it's part of a section.

You can't avoid things from being put to the wrong place in all cases. While the end-of-file is where things obviously helps, you can always put things in the middle of file to the wrong section.

It improves readability of generated doxygen.

Is having fully complete and readable and useful doxugen is the end goal?

Here are some disadvantages.

Visual noise

/* -------------------------------------------------------------------- */
/** \name Title for Code Section
 * \{ */

There are two things here. There are two comments here, which also leads to funky * ** * vertical sequence.
For the reader the \name and \{ is just the noise.

Verbosity of the description

While it isn't intrinsic to Doxygen, the proposal lacks explanation of how and where to put more elaborate description of the section. Something what is in natural comment style would be:

/* --------------------------------------------------------------------
 * Section which does calculate FOO.
 *
 * All the inputs here are in object space, the outputs are in happyspace.
 */

Where does this belong in the proposal.

More things to worry about

Every so often things gets refactored, reshuffled, moved across files so section might no longer exist in the file. It's easy to forget remove/add the end-of-section marker.

Decreases readability a lot

More like a screenshot to visualize the noise paragraph above.

Natural	Doxygen

Sure, sections in headers aren't so common. It doesn't make them unneeded.
And something which becomes official enforcement in the guidelines should survive basic usecases.

This proposal is to add doxy sections to the style guide, something like this.

I do not support this. Not in the current form anwyay.

What can be added is a style of how to make non-doxy section. Which could be

/* --------------------------------------------------------------------
 * Name of the section.
 *
 * Optional description.
 */

The closing */ might be moved to the previous line.

Pros:

Minimal visual noise, easy to read.
Looks almost like proposed Doxygen.
Allows to be used in the headers example above.
Still standardizes sections definition syntax.

Cons:

Doesn't follow lets-doxygen-everything.
Doesn't have explicit end marker.

To be clear: this is not a proposal to have only non-Doxygen comments. There are places where Doxygen style is actually helping (with the markers i.e.). But there are plenty cases where pure natural comment works way better.

Both things can be available in a developers toolboxes, and the one which works best is used in that specific instance.

While i agree having some standard way to define sections is useful, the doxygen have big downsides to me. > All Blender code uses the same convention for sections. You can have consistency for section with whatever-style. > Developers can use utilities to navigate between sections - which work across all files. There is nothing really ready-to-go for this in any IDE. For the cusomization scripts you can train them to use whatever-regex. > Beginning and sections make it clear when adding code to the bottom of a file that it's part of a section. You can't avoid things from being put to the wrong place in all cases. While the end-of-file is where things obviously helps, you can always put things in the middle of file to the wrong section. > It improves readability of generated doxygen. Is having fully complete and readable and useful doxugen is the end goal? ---- Here are some disadvantages. **Visual noise** ``` /* -------------------------------------------------------------------- */ /** \name Title for Code Section * \{ */ ``` There are two things here. There are two comments here, which also leads to funky * ** * vertical sequence. For the reader the `\name` and `\{` is just the noise. **Verbosity of the description** While it isn't intrinsic to Doxygen, the proposal lacks explanation of how and where to put more elaborate description of the section. Something what is in natural comment style would be: ``` /* -------------------------------------------------------------------- * Section which does calculate FOO. * * All the inputs here are in object space, the outputs are in happyspace. */ ``` Where does this belong in the proposal. **More things to worry about** Every so often things gets refactored, reshuffled, moved across files so section might no longer exist in the file. It's easy to forget remove/add the end-of-section marker. **Decreases readability a lot** More like a screenshot to visualize the noise paragraph above. | Natural | Doxygen | | -- | -- | | ![antural.png](https://archive.blender.org/developer/F8412088/antural.png) | ![doxygen.png](https://archive.blender.org/developer/F8412090/doxygen.png) | Sure, sections in headers aren't so common. It doesn't make them unneeded. And something which becomes official enforcement in the guidelines should survive basic usecases. --- > This proposal is to add doxy sections to the style guide, something like this. I do not support this. Not in the current form anwyay. What can be added is a style of how to make non-doxy section. Which could be ``` /* -------------------------------------------------------------------- * Name of the section. * * Optional description. */ ``` *The closing `*/` might be moved to the previous line.* Pros: - Minimal visual noise, easy to read. - Looks almost like proposed Doxygen. - Allows to be used in the headers example above. - Still standardizes sections definition syntax. Cons: - Doesn't follow lets-doxygen-everything. - Doesn't have explicit end marker. To be clear: this is not a proposal to have only non-Doxygen comments. There are places where Doxygen style is actually helping (with the markers i.e.). But there are plenty cases where pure natural comment works way better. Both things can be available in a developers toolboxes, and the one which works best is used in that specific instance.

Campbell Barton commented

2020-03-18 00:02:18 +01:00

In #74845#892592, @Sergey wrote:
While i agree having some standard way to define sections is useful, the doxygen have big downsides to me.

Even if you can rationalize doxygen sections being worse in some way,
I can't see how you can claim these are big downsides.

All Blender code uses the same convention for sections.

You can have consistency for section with whatever-style.

As we already use doxygen for structured comments, there should be strong justification to invent a different comment style.

Something that should be agreed on, documented and applied to existing code.

Developers can use utilities to navigate between sections - which work across all files.

There is nothing really ready-to-go for this in any IDE. For the cusomization scripts you can train them to use whatever-regex.

While true, that would mean defining multiple regex patterns, since it would need to navigate doxygen as well as whatever else is used in that file.

Beginning and sections make it clear when adding code to the bottom of a file that it's part of a section.

You can't avoid things from being put to the wrong place in all cases. While the end-of-file is where things obviously helps, you can always put things in the middle of file to the wrong section.

The suggestion to use our own add-hoc structure ignores that this has been done and proven to be poorly maintained.

Some files have headings used in the middle of a file, anyone editing the file might not think to check.

Even without any doxygen IDE integration you can search for \{ to jump between sections.

In practice if you check files that use doxygen section, they tend to be better organized:

Check:

./source/blender/render/intern/source/pipeline.c
./source/blender/render/intern/source/multires_bake.c
./source/blender/blenkernel/intern/armature.c

The sections get outdated, when adding new code it's hard to tell if someone forgot to end a section, or the functions there are in fact part of the section.
Sometimes developers feel the need to add ***END XXX***, to make better sense of whats going on.

This tends to be applied in a haphazard fashion, with the added down-side that the reader doesn't know which sections they should expect to see ending:

./source/blender/blenkernel/intern/armature.c:1798:0:
/* ************END Armature Deform******************* */
./source/blender/editors/include/ED_view3d.h:278:0:
/* ***end iterators*** */
./source/blender/editors/space_api/spacetypes.c:336:0:
/* ******************************end template*********************** */
./source/blender/editors/space_console/space_console.c:204:0:
/* *************end drop*********** */
./source/blender/editors/space_node/space_node.c:687:0:
/* *************end drop*********** */
./source/blender/editors/space_sequencer/space_sequencer.c:452:0:
/* *************end drop*********** */
./source/blender/editors/space_text/space_text.c:376:0:
/* *************end drop*********** */
./source/blender/makesdna/intern/dna_genfile.c:323:0:
/* *************************END DIV********************** */
./source/blender/makesdna/intern/dna_genfile.c:618:0:
/* ********************END READ DNA********************** */

It improves readability of generated doxygen.

Is having fully complete and readable and useful doxugen is the end goal?

As noted in the original statement, this is only a small 'pro' as we're currently not relying on generted doxygen docs,
nevertheless, it means we can generate better doxygen docs which may prove useful in the future.

In #74845#892592, @Sergey wrote:
Decreases readability a lot

More like a screenshot to visualize the noise paragraph above.

| Natural | Doxygen |
| | |

Sure, sections in headers aren't so common. It doesn't make them unneeded.
And something which becomes official enforcement in the guidelines should survive basic usecases.

The example above is not representative of how sections are typically used.

There are almost never a single section per function.
They aren't often used for headers, unless there are larger areas which benefit from grouping.

Examples of headers that use doxygen are:

./source/blender/makesdna/DNA_meshdata_types.h
./source/blender/makesdna/DNA_space_types.h
./source/blender/blenlib/BLI_utildefines.h

This is an example of more typical use:

{F8412895}

In #74845#892592, @Sergey wrote:
/* --------------------------------------------------------------------
 * Name of the section.
 *
 * Optional description.
 */
The closing */ might be moved to the previous line.

Pros:

Minimal visual noise, easy to read.

Looks almost like proposed Doxygen.

Allows to be used in the headers example above.

Still standardizes sections definition syntax.

Cons:

Doesn't follow lets-doxygen-everything.

Doesn't have explicit end marker.

To be clear: this is not a proposal to have only non-Doxygen comments. There are places where Doxygen style is actually helping (with the markers i.e.). But there are plenty cases where pure natural comment works way better.

Both things can be available in a developers toolboxes, and the one which works best is used in that specific instance.

The main con with this suggestion is it means we have different code-style for sections based on the preference of the developer who last worked on a piece of code.

If you edit one developers code, they complain if you use doxy sections, if you edit another developers, they complain if you don't.

It's better to avoid the whole code-style-ownership issue we had before switching to a consistent code style.

> In #74845#892592, @Sergey wrote: > While i agree having some standard way to define sections is useful, the doxygen have big downsides to me. Even if you can rationalize doxygen sections being worse in some way, I can't see how you can claim these are *big downsides*. >> All Blender code uses the same convention for sections. > > You can have consistency for section with whatever-style. As we already use doxygen for structured comments, there should be strong justification to invent a different comment style. Something that should be agreed on, documented and applied to existing code. >> Developers can use utilities to navigate between sections - which work across all files. > > There is nothing really ready-to-go for this in any IDE. For the cusomization scripts you can train them to use whatever-regex. While true, that would mean defining multiple regex patterns, since it would need to navigate doxygen as well as whatever else is used in that file. >> Beginning and sections make it clear when adding code to the bottom of a file that it's part of a section. > > You can't avoid things from being put to the wrong place in all cases. While the end-of-file is where things obviously helps, you can always put things in the middle of file to the wrong section. The suggestion to use our own add-hoc structure ignores that this has been done and proven to be poorly maintained. Some files have headings used in the middle of a file, anyone editing the file might not think to check. Even without any doxygen IDE integration you can search for `\{` to jump between sections. In practice if you check files that use doxygen section, they tend to be better organized: Check: - ./source/blender/render/intern/source/pipeline.c - ./source/blender/render/intern/source/multires_bake.c - ./source/blender/blenkernel/intern/armature.c The sections get outdated, when adding new code it's hard to tell if someone forgot to end a section, or the functions there are in fact part of the section. Sometimes developers feel the need to add ` ***END XXX*** `, to make better sense of whats going on. This tends to be applied in a haphazard fashion, with the added down-side that the reader doesn't know which sections they should expect to see ending: ``` ./source/blender/blenkernel/intern/armature.c:1798:0: /* ************END Armature Deform******************* */ ./source/blender/editors/include/ED_view3d.h:278:0: /* ***end iterators*** */ ./source/blender/editors/space_api/spacetypes.c:336:0: /* ******************************end template*********************** */ ./source/blender/editors/space_console/space_console.c:204:0: /* *************end drop*********** */ ./source/blender/editors/space_node/space_node.c:687:0: /* *************end drop*********** */ ./source/blender/editors/space_sequencer/space_sequencer.c:452:0: /* *************end drop*********** */ ./source/blender/editors/space_text/space_text.c:376:0: /* *************end drop*********** */ ./source/blender/makesdna/intern/dna_genfile.c:323:0: /* *************************END DIV********************** */ ./source/blender/makesdna/intern/dna_genfile.c:618:0: /* ********************END READ DNA********************** */ ``` ---- >> It improves readability of generated doxygen. > > Is having fully complete and readable and useful doxugen is the end goal? As noted in the original statement, this is only a small 'pro' as we're currently not relying on generted doxygen docs, nevertheless, it means we can generate better doxygen docs which may prove useful in the future. ---- > In #74845#892592, @Sergey wrote: > **Decreases readability a lot** > > More like a screenshot to visualize the noise paragraph above. > > | Natural | Doxygen | > | ![antural.png](https://archive.blender.org/developer/F8412088/antural.png) | ![doxygen.png](https://archive.blender.org/developer/F8412090/doxygen.png) | > > Sure, sections in headers aren't so common. It doesn't make them unneeded. > And something which becomes official enforcement in the guidelines should survive basic usecases. The example above is not representative of how sections are typically used. - There are almost never a single section per function. - They aren't often used for headers, unless there are larger areas which benefit from grouping. ``` Examples of headers that use doxygen are: ``` - ./source/blender/makesdna/DNA_meshdata_types.h - ./source/blender/makesdna/DNA_space_types.h - ./source/blender/blenlib/BLI_utildefines.h ``` This is an example of more typical use: ``` ``` {F8412895} ``` ---- > In #74845#892592, @Sergey wrote: > > ``` > /* -------------------------------------------------------------------- > * Name of the section. > * > * Optional description. > */ > ``` > > *The closing `*/` might be moved to the previous line.* > > Pros: > - Minimal visual noise, easy to read. > - Looks almost like proposed Doxygen. > - Allows to be used in the headers example above. > - Still standardizes sections definition syntax. > > Cons: > - Doesn't follow lets-doxygen-everything. > - Doesn't have explicit end marker. > > To be clear: this is not a proposal to have only non-Doxygen comments. There are places where Doxygen style is actually helping (with the markers i.e.). But there are plenty cases where pure natural comment works way better. > > Both things can be available in a developers toolboxes, and the one which works best is used in that specific instance. The main con with this suggestion is it means we have different code-style for sections based on the preference of the developer who last worked on a piece of code. If you edit one developers code, they complain if you use doxy sections, if you edit another developers, they complain if you don't. It's better to avoid the whole code-style-ownership issue we had before switching to a consistent code style.

Evan Wilson commented

2020-03-18 00:39:52 +01:00

Added subscriber: @EAW

Sergey Sharybin commented

2020-03-18 11:38:49 +01:00

As we already use doxygen for structured comments, there should be strong justification to invent a different comment style.

That is true. I agree when the structured comment is needed doxygen is to be used.

However, there are also cases in the existing code where I believe doxygen adds too much noise and hurts readability. For me this is a strong enough reason to pursuit something different, such as what I proposed. It may comes down to personal preference. But that argument works both ways.

The suggestion to use our own add-hoc structure ignores that this has been done and proven to be poorly maintained.

I did not suggest use own add-hoc style. You read proposal as: if having the doxygen markers hurts readability just don't use them. The rest look and feel of the section is exactly the same (same opening /** ------- , same * in the beginning of the line, same new line for closing */).

Effectively, applying the The Rule Number Two of our guide:

Strive for clarity, even if that means occasionally breaking the guidelines. Use your head and ask for advice if your common sense seems to disagree 
with the conventions.

While true, that would mean defining multiple regex patterns, since it would need to navigate doxygen as well as whatever else is used in that file.

It is possible to set regex to /** -{72} (adjust number of repetitions according to the final proposal).

Check:
./source/blender/render/intern/source/pipeline.c
./source/blender/render/intern/source/multires_bake.c
./source/blender/blenkernel/intern/armature.c

Examples coming from legacy code are a hard sell. Everyone agrees they are ugly, hard-to-manage-clutter, needs-to-be-restructured and needs-to-be-refatored.

There is a reason why something is not currently enforced. But what happens: one person goes and from own will tries to push his own view, wasting own time, wasting time of other active developer. It would way better time investment to go and address root of the cause, not the symptoms.

There is also one of the reason why quality days were introduced. And in fact all the cases you've listed are listed as projects.

The example above is not representative of how sections are typically used.

Typically in which sense? Typically in new code or in the scope of legacy codebase?

There are almost never a single section per function.

The code starts small, then it grows. Not all the code comes from years of legacy and mess.
And, hopefully, doesn't grow to the mess.

They aren't often used for headers, unless there are larger areas which benefit from grouping.

The fact that they are not used in headers makes headers a huge mess. In almost any header in BKE for example.

To me this again sounds like addressing symptom rather than a cause: "Something isn't currently used, so we can just ignore this specific usecase look ugly!" instead of "Oh, wait a minute, the same thing can be applied to lots of existing code and it will help. Although this annoying thing which looked very isolated is now spreading into many areas. Can we avoid this annoyance? How?"

> As we already use doxygen for structured comments, there should be strong justification to invent a different comment style. That is true. I agree when the structured comment is needed doxygen is to be used. However, there are also cases in the existing code where I believe doxygen adds too much noise and hurts readability. For me this is a strong enough reason to pursuit something different, such as what I proposed. It may comes down to personal preference. But that argument works both ways. > The suggestion to use our own add-hoc structure ignores that this has been done and proven to be poorly maintained. I did not suggest use own add-hoc style. You read proposal as: if having the doxygen markers hurts readability just don't use them. The rest look and feel of the section is exactly the same (same opening `/** ------- `, same `*` in the beginning of the line, same new line for closing `*/`). Effectively, applying the The Rule Number Two of our guide: ``` Strive for clarity, even if that means occasionally breaking the guidelines. Use your head and ask for advice if your common sense seems to disagree with the conventions. ``` > While true, that would mean defining multiple regex patterns, since it would need to navigate doxygen as well as whatever else is used in that file. It is possible to set regex to `/** -{72}` (adjust number of repetitions according to the final proposal). > Check: > ./source/blender/render/intern/source/pipeline.c > ./source/blender/render/intern/source/multires_bake.c > ./source/blender/blenkernel/intern/armature.c Examples coming from legacy code are a hard sell. Everyone agrees they are ugly, hard-to-manage-clutter, needs-to-be-restructured and needs-to-be-refatored. There is a reason why something is not currently enforced. But what happens: one person goes and from own will tries to push his own view, wasting own time, wasting time of other active developer. It would way better time investment to go and address root of the cause, not the symptoms. There is also one of the reason why quality days were introduced. And in fact all the cases you've listed are listed as projects. > The example above is not representative of how sections are typically used. Typically in which sense? Typically in new code or in the scope of legacy codebase? > There are almost never a single section per function. The code starts small, then it grows. Not all the code comes from years of legacy and mess. And, hopefully, doesn't grow to the mess. > They aren't often used for headers, unless there are larger areas which benefit from grouping. The fact that they are not used in headers makes headers a huge mess. In almost any header in BKE for example. To me this again sounds like addressing symptom rather than a cause: "Something isn't currently used, so we can just ignore this specific usecase look ugly!" instead of "Oh, wait a minute, the same thing can be applied to lots of existing code and it will help. Although this annoying thing which looked very isolated is now spreading into many areas. Can we avoid this annoyance? How?"

Campbell Barton commented

2020-03-18 12:17:50 +01:00

Probably we have to agree to disagree on this readability issue, to me the difference is very minor - if any.

Compare this to the significant hit in readability from clang-format doing strange things with previously well formatted blocks of code.

Sometimes it's worth going with a "good-enough" convention, even if you can pick examples where it's not great.

In general it works well enough for C/C++ source files which is where it's mostly being used, your point is more reasonable for headers.

Why not use doxy sections for source files, optionally light-weight sections for headers (if doxy feels too dense to the author).

In #74845#893321, @Sergey wrote:

As we already use doxygen for structured comments, there should be strong justification to invent a different comment style.

That is true. I agree when the structured comment is needed doxygen is to be used.

However, there are also cases in the existing code where I believe doxygen adds too much noise and hurts readability. For me this is a strong enough reason to pursuit something different, such as what I proposed. It may comes down to personal preference. But that argument works both ways.

The suggestion to use our own add-hoc structure ignores that this has been done and proven to be poorly maintained.

I did not suggest use own add-hoc style. You read proposal as: if having the doxygen markers hurts readability just don't use them. The rest look and feel of the section is exactly the same (same opening /** ------- , same * in the beginning of the line, same new line for closing */).

Effectively, applying the The Rule Number Two of our guide:
Strive for clarity, even if that means occasionally breaking the guidelines. Use your head and ask for advice if your common sense seems to disagree 
with the conventions.
While true, that would mean defining multiple regex patterns, since it would need to navigate doxygen as well as whatever else is used in that file.

It is possible to set regex to /** -{72} (adjust number of repetitions according to the final proposal).

It's not so simple if you want to extract the title from both kinds of sections.

Check:
./source/blender/render/intern/source/pipeline.c
./source/blender/render/intern/source/multires_bake.c
./source/blender/blenkernel/intern/armature.c

Examples coming from legacy code are a hard sell. Everyone agrees they are ugly, hard-to-manage-clutter, needs-to-be-restructured and needs-to-be-refatored.

There is a reason why something is not currently enforced. But what happens: one person goes and from own will tries to push his own view, wasting own time, wasting time of other active developer. It would way better time investment to go and address root of the cause, not the symptoms.

There is also one of the reason why quality days were introduced. And in fact all the cases you've listed are listed as projects.

There are examples of this that don't use legacy code:

./blender/gpu/intern/gpu_codegen.c has a single title in the file, it's not clear when the section ends.
./blender/editors/space_file/filelist.c uses different kinds of section syntax with a Main section at the end that seems to include a lot of unrelated things.
./blender/editors/space_file/file_ops.c has two sections, from a brief look it seems there should be more (file-delete operator is listed under bookmarks).

Even accepting the legacy code refactoring, the person who refactors them will need to update the sections, either keeping what is there or use some other convention.

The example above is not representative of how sections are typically used.

Typically in which sense? Typically in new code or in the scope of legacy codebase?

Not typical as in, you would typically either document each function, or group many functions, rarely using sections for multiple one-off functions.

There are almost never a single section per function.

The code starts small, then it grows. Not all the code comes from years of legacy and mess.
And, hopefully, doesn't grow to the mess.

They aren't often used for headers, unless there are larger areas which benefit from grouping.

The fact that they are not used in headers makes headers a huge mess. In almost any header in BKE for example.

True, although managing mess in header's isn't as big a problem in practice then messy source files over 1k in length, where grouping functions is often done poorly.

To me this again sounds like addressing symptom rather than a cause: "Something isn't currently used, so we can just ignore this specific usecase look ugly!" instead of "Oh, wait a minute, the same thing can be applied to lots of existing code and it will help. Although this annoying thing which looked very isolated is now spreading into many areas. Can we avoid this annoyance? How?"

Conventions are like this - again, like clang-format, we pick a convention which isn't always perfect, but good-enough most of the time. And we don't have to quibble about when to use it.

Probably we have to agree to disagree on this readability issue, to me the difference is very minor - if any. Compare this to the significant hit in readability from clang-format doing strange things with previously well formatted blocks of code. Sometimes it's worth going with a "good-enough" convention, even if you can pick examples where it's not great. In general it works well enough for C/C++ source files which is where it's mostly being used, your point is more reasonable for headers. Why not use doxy sections for source files, optionally light-weight sections for headers (if doxy feels too dense to the author). ---- > In #74845#893321, @Sergey wrote: >> As we already use doxygen for structured comments, there should be strong justification to invent a different comment style. > > That is true. I agree when the structured comment is needed doxygen is to be used. > > However, there are also cases in the existing code where I believe doxygen adds too much noise and hurts readability. For me this is a strong enough reason to pursuit something different, such as what I proposed. It may comes down to personal preference. But that argument works both ways. > >> The suggestion to use our own add-hoc structure ignores that this has been done and proven to be poorly maintained. > > I did not suggest use own add-hoc style. You read proposal as: if having the doxygen markers hurts readability just don't use them. The rest look and feel of the section is exactly the same (same opening `/** ------- `, same `*` in the beginning of the line, same new line for closing `*/`). > > Effectively, applying the The Rule Number Two of our guide: > > ``` > Strive for clarity, even if that means occasionally breaking the guidelines. Use your head and ask for advice if your common sense seems to disagree > with the conventions. > ``` > >> While true, that would mean defining multiple regex patterns, since it would need to navigate doxygen as well as whatever else is used in that file. > > It is possible to set regex to `/** -{72}` (adjust number of repetitions according to the final proposal). It's not so simple if you want to extract the title from both kinds of sections. >> Check: >> ./source/blender/render/intern/source/pipeline.c >> ./source/blender/render/intern/source/multires_bake.c >> ./source/blender/blenkernel/intern/armature.c > > Examples coming from legacy code are a hard sell. Everyone agrees they are ugly, hard-to-manage-clutter, needs-to-be-restructured and needs-to-be-refatored. > > There is a reason why something is not currently enforced. But what happens: one person goes and from own will tries to push his own view, wasting own time, wasting time of other active developer. It would way better time investment to go and address root of the cause, not the symptoms. > > There is also one of the reason why quality days were introduced. And in fact all the cases you've listed are listed as projects. There are examples of this that don't use legacy code: - `./blender/gpu/intern/gpu_codegen.c` has a single title in the file, it's not clear when the section ends. - `./blender/editors/space_file/filelist.c` uses different kinds of section syntax with a `Main` section at the end that seems to include a lot of unrelated things. - `./blender/editors/space_file/file_ops.c` has two sections, from a brief look it seems there should be more (file-delete operator is listed under bookmarks). Even accepting the legacy code refactoring, the person who refactors them will need to update the sections, either keeping what is there or use some other convention. >> The example above is not representative of how sections are typically used. > > Typically in which sense? Typically in new code or in the scope of legacy codebase? Not typical as in, you would typically either document each function, or group many functions, rarely using sections for multiple one-off functions. >> There are almost never a single section per function. > > The code starts small, then it grows. Not all the code comes from years of legacy and mess. > And, hopefully, doesn't grow to the mess. > >> They aren't often used for headers, unless there are larger areas which benefit from grouping. > > The fact that they are not used in headers makes headers a huge mess. In almost any header in BKE for example. True, although managing mess in header's isn't as big a problem in practice then messy source files over 1k in length, where grouping functions is often done poorly. > To me this again sounds like addressing symptom rather than a cause: "Something isn't currently used, so we can just ignore this specific usecase look ugly!" instead of "Oh, wait a minute, the same thing can be applied to lots of existing code and it will help. Although this annoying thing which looked very isolated is now spreading into many areas. Can we avoid this annoyance? How?" Conventions are like this - again, like clang-format, we pick a convention which isn't always perfect, but good-enough most of the time. And we don't have to quibble about when to use it.

Sergey Sharybin commented

2020-03-25 15:08:15 +01:00

Compare this to the significant hit in readability from clang-format doing strange things with previously well formatted blocks of code.

I am yet to see non-legacy and nicely designed code which readability became worse after applying clang-format.
And even if such case exist, it's always possible to clang-format off.

We also spent a lot of time refining the config to make everyone ok with the change.

In general it works well enough for C/C++ source files which is where it's mostly being used, your point is more reasonable for headers.

How about Python?

Why not use doxy sections for source files, optionally light-weight sections for headers (if doxy feels too dense to the author).

Not sure what you mean by light-weight. Is it the

/* --------------------------------------------------------------------
 * Name of the section.
 */

?

Think that would be a compromise we both could live with.
Any patch you have handy to show how it'll make the infamous multires_reshape to look like?

There are examples of this that don't use legacy code:

Although, the code structured in exactly legacy manner.

True, although managing mess in header's isn't as big a problem in practice then messy source files over 1k in length, where grouping functions is often done poorly.

To me it's exactly other way around: navigating in the source (definition) file is easy, since you there from either public API or from a function call.
It is a public API which is a huge mess (and this is something where you expect a lot of clarity to pick up the right function to use, or know that it exists, know where new function belongs to).

What's also weird is why it's only two of us? Is that enough for decision making? Or feedback of more core developers is essential?

> Compare this to the significant hit in readability from clang-format doing strange things with previously well formatted blocks of code. I am yet to see non-legacy and nicely designed code which readability became worse after applying clang-format. And even if such case exist, it's always possible to `clang-format off`. We also spent a lot of time refining the config to make everyone ok with the change. > In general it works well enough for C/C++ source files which is where it's mostly being used, your point is more reasonable for headers. How about Python? > Why not use doxy sections for source files, optionally light-weight sections for headers (if doxy feels too dense to the author). Not sure what you mean by light-weight. Is it the ``` /* -------------------------------------------------------------------- * Name of the section. */ ``` ? Think that would be a compromise we both could live with. Any patch you have handy to show how it'll make the infamous multires_reshape to look like? ---- > There are examples of this that don't use legacy code: Although, the code structured in exactly legacy manner. > True, although managing mess in header's isn't as big a problem in practice then messy source files over 1k in length, where grouping functions is often done poorly. To me it's exactly other way around: navigating in the source (definition) file is easy, since you there from either public API or from a function call. It is a public API which is a huge mess (and this is something where you expect a lot of clarity to pick up the right function to use, or know that it exists, know where new function belongs to). ---- What's also weird is why it's only two of us? Is that enough for decision making? Or feedback of more core developers is essential?

Campbell Barton commented

2020-04-01 03:08:01 +02:00

Since writing this proposal I'm even more convinced that using over-lines for comments as sections isn't easily maintainable.

Even in new a new medium sized file with 3 operators, someone adds a section, but developers adding new code forgets to do the same. 9b0d72aa3d

Other replies inline.

In #74845#897650, @Sergey wrote:

Compare this to the significant hit in readability from clang-format doing strange things with previously well formatted blocks of code.

I am yet to see non-legacy and nicely designed code which readability became worse after applying clang-format.

Mostly these are related to argument alignment & grouping arguments when it makes sense to - see: P1319

In #74845#897650, @Sergey wrote:
And even if such case exist, it's always possible to clang-format off.

In general I've been avoiding this with very few exceptions, we could use it in cases of worse readability though.

In #74845#897650, @Sergey wrote:
We also spent a lot of time refining the config to make everyone ok with the change.

There was quite a bit of disagreement too, even choice of 2 spaces was in part from a limitation in clang-format (which has since been resolved ).

In #74845#897650, @Sergey wrote:

In general it works well enough for C/C++ source files which is where it's mostly being used, your point is more reasonable for headers.

How about Python?

Would rather separate the discussion, but sure - am open to applying this to Python too.

In #74845#897650, @Sergey wrote:

Why not use doxy sections for source files, optionally light-weight sections for headers (if doxy feels too dense to the author).

Not sure what you mean by light-weight. Is it the
/* --------------------------------------------------------------------
 * Name of the section.
 */
?

I don't have a strong opinion on this - sure it seems fine.

Think that would be a compromise we both could live with.
Any patch you have handy to show how it'll make the infamous multires_reshape to look like?

There are examples of this that don't use legacy code:

Although, the code structured in exactly legacy manner.

Since it's new code being committed it shouldn't be discounted.

True, although managing mess in header's isn't as big a problem in practice then messy source files over 1k in length, where grouping functions is often done poorly.

To me it's exactly other way around: navigating in the source (definition) file is easy, since you there from either public API or from a function call.
It is a public API which is a huge mess (and this is something where you expect a lot of clarity to pick up the right function to use, or know that it exists, know where new function belongs to).

Put it differently, if you're adding a new declaration into a well organized header, it's fairly easy to add it in a logical place.

If you're adding new code into a large source file, it's not always immediately obvious where it might go, so developers often add it to the top/bottom.

What's also weird is why it's only two of us? Is that enough for decision making? Or feedback of more core developers is essential?

Not sure, although it seems we've nearly come to an agreement, why not tweak proposal and check with other devs.

Since writing this proposal I'm even more convinced that using over-lines for comments as sections isn't easily maintainable. Even in new a new medium sized file with 3 operators, someone adds a section, but developers adding new code forgets to do the same. 9b0d72aa3d Other replies inline. ---- > In #74845#897650, @Sergey wrote: >> Compare this to the significant hit in readability from clang-format doing strange things with previously well formatted blocks of code. > > I am yet to see non-legacy and nicely designed code which readability became worse after applying clang-format. Mostly these are related to argument alignment & grouping arguments when it makes sense to - see: [P1319](https://archive.blender.org/developer/P1319.txt) > In #74845#897650, @Sergey wrote: > And even if such case exist, it's always possible to `clang-format off`. In general I've been avoiding this with very few exceptions, we could use it in cases of worse readability though. > In #74845#897650, @Sergey wrote: > We also spent a lot of time refining the config to make everyone ok with the change. There was quite a bit of disagreement too, even choice of 2 spaces was in part from a limitation in clang-format (which has [since been resolved ](https://reviews.llvm.org/D68296)). > In #74845#897650, @Sergey wrote: >> In general it works well enough for C/C++ source files which is where it's mostly being used, your point is more reasonable for headers. > > How about Python? Would rather separate the discussion, but sure - am open to applying this to Python too. > In #74845#897650, @Sergey wrote: > >> Why not use doxy sections for source files, optionally light-weight sections for headers (if doxy feels too dense to the author). > > Not sure what you mean by light-weight. Is it the > > ``` > /* -------------------------------------------------------------------- > * Name of the section. > */ > ``` > ? > I don't have a strong opinion on this - sure it seems fine. > Think that would be a compromise we both could live with. > Any patch you have handy to show how it'll make the infamous multires_reshape to look like? > >> There are examples of this that don't use legacy code: > > Although, the code structured in exactly legacy manner. Since it's new code being committed it shouldn't be discounted. >> True, although managing mess in header's isn't as big a problem in practice then messy source files over 1k in length, where grouping functions is often done poorly. > > To me it's exactly other way around: navigating in the source (definition) file is easy, since you there from either public API or from a function call. > It is a public API which is a huge mess (and this is something where you expect a lot of clarity to pick up the right function to use, or know that it exists, know where new function belongs to). Put it differently, if you're adding a new declaration into a well organized header, it's fairly easy to add it in a logical place. If you're adding new code into a large source file, it's not always immediately obvious where it might go, so developers often add it to the top/bottom. > ---- > > What's also weird is why it's only two of us? Is that enough for decision making? Or feedback of more core developers is essential? Not sure, although it seems we've nearly come to an agreement, why not tweak proposal and check with other devs.

Brecht Van Lommel commented

2020-04-08 14:20:50 +02:00

Added subscriber: @brecht

Brecht Van Lommel commented

2020-04-08 14:20:50 +02:00

+1 from me.

Sergey Sharybin commented

2020-04-08 15:07:31 +02:00

@ideasman42, think we agreed already. But i'm curious about you thoughs about:

Current proposal:

/* -------------------------------------------------------------------- */
/** \name Title of Code Section

Explain in more detail the purpose of the section.
{ */

...
/** \} */

Possible modification:

/* -------------------------------------------------------------------- */
/** \name Title of Code Section

Explain in more detail the purpose of the section.
{ */

...
/** \} */

Is more vertical space, but kind of makes detailed description easier to read. Which could be really nice if one desires to add code snipped to demonstrate usecase or something like this.

@ideasman42, think we agreed already. But i'm curious about you thoughs about: Current proposal: ``` /* -------------------------------------------------------------------- */ /** \name Title of Code Section ``` * * Explain in more detail the purpose of the section. * \{ */ ``` ... /** \} */ ``` Possible modification: ``` /* -------------------------------------------------------------------- */ /** \name Title of Code Section ``` * * Explain in more detail the purpose of the section. * * \{ */ ``` ... /** \} */ ``` Is more vertical space, but kind of makes detailed description easier to read. Which could be really nice if one desires to add code snipped to demonstrate usecase or something like this.

Campbell Barton commented

2020-04-08 15:26:49 +02:00

In general I'd rater use less vertical space, since this is already a bit heavy on vertical space.

If there are large blocks of code, maybe it makes sense in some cases.

If this is done it could be some exceptional case - I don't mind, in general cases I'd rather avoid.

Example with doxygen code-block without a space:

/* -------------------------------------------------------------------- */
/** \name Title of Code Section
 *
 * Explain in more detail the purpose of the section.
 *
 * \code{.c}
 * for (int i = 0; i < len; i++){
 *   project_v3_v3v3(c, v, v_plane);
 *   sub_v3_v3v3(c, v, c);
 * }
 * \endcode
 * \{ */

In general I'd rater use less vertical space, since this is already a bit heavy on vertical space. If there are large blocks of code, maybe it makes sense in some cases. If this is done it could be some exceptional case - I don't mind, in general cases I'd rather avoid. Example with doxygen code-block without a space: ``` /* -------------------------------------------------------------------- */ /** \name Title of Code Section * * Explain in more detail the purpose of the section. * * \code{.c} * for (int i = 0; i < len; i++){ * project_v3_v3v3(c, v, v_plane); * sub_v3_v3v3(c, v, c); * } * \endcode * \{ */ ```

Campbell Barton commented

2020-04-17 03:54:19 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Campbell Barton closed this issue

2020-04-17 03:54:19 +02:00

Campbell Barton self-assigned this 2020-04-17 03:54:19 +02:00

Campbell Barton commented

2020-04-17 03:54:19 +02:00

Updated our code style docs and added the proposed text:

https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comment_Sections

Updated our code style docs and added the proposed text: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comment_Sections

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Policy for style guide: doxygen sections #74845

Where non-doxygen Sections are acceptable.