Code Style: documentation at declaration or definition #92709
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
8 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#92709
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The API docs section in the C/C++ style guide dictates that interface documentation should be part of the implementation file, and*Comments in header are optional but keep brief//.
Note that in this proposal I'm not writing about APIs, because we do not have a clear definition of what an API is. For me it's more important to talk about the public interface of a module, where "module" means "combination of header file and implementation file(s)".
Advantages of the current guideline:
Disadvantages of the current guideline:
static
functions are declared before they are used, putting the lowest-level functions before the higher-level main code (when reading top to bottom).Proposal:
I think that this change addresses all the disadvantages listed above. Since the order of symbols in the header file is less important for the compiler than in the implementation file, the header file can be ordered to make sense to human readers. Having the interface documentation there makes it easier to discover, and also easier to write (because it has a clearer focus on functionality/meaning rather than implementation details).
Update: After the below discussion (up to and including 2021-11-11), this is the proposed new guideline:
As for placement of documentation, follow these guidelines:
When there is overlap between internal and public functions, for example when two public functions actually call an internal function with some additional parameters, the internal function's documentation can refer to the public function. That way documentation doesn't have to be copied between those.
In Summary:
*_internal.h
headers..c
file), only use formal parameter and return documentation (\param
and\return
) to the public doc-string. Doxygen cannot deal with having those defined twice in different files.Changed status from 'Needs Triage' to: 'Confirmed'
Added subscriber: @dr.sybren
Added subscriber: @GeorgiaPacific
Added subscriber: @howardt
This proposal makes sense to me.
Added subscriber: @ideasman42
There are some issues I noticed with keeping code-comments in headers.
Developers would make changes to the code and not updated the headers ... or note important details to the implementation (which should have been in the public doc-string).
Duplicate/overlapping doc-strings. More than once I ran into situations where both header and implementation had different doc-strings (not different in public/implementation sense), I needed to use SVN annotations to figure out which comment was more up to date or how comments would be merged into a single comment.
Note that it seems likely some of this doubling up on docs was caused by developers not using editors that support easily jumping to declarations.
There are some other advantages in having comprehensive docs in the code.
If everyone is committed to making a different convention work, it could be fine.
Just noting some pitfalls - given the number of issues in our current doc-strings (documentation for parameters that don't exist - to pick a simple example) it's possible for these kinds of issues to bite us again.
Details to Consider:
example.hh
example.cc
Added subscriber: @JulianEisel
Another thing to consider is functions without declarations in headers. These should also have a design that should be explained in a function comment. But since they don't have a declaration in a header, do we just put them in the source file (unlike other function design comments?).
(This is not to argue against the guidline change, just mentioning as something to consider.)
Edit: I see this is mentioned in the proposal:
This is not always as straightforward as you might expect.
In some cases a static function is called by a public function with minor differences (typically fewer arguments).
Take
ui_handle_afterfunc_add_operator_ex
for example, directly below it isui_handle_afterfunc_add_operator
which calls it, currently the static function has the doc-string.If we document both
ui_handle_afterfunc_add_operator_ex
andui_handle_afterfunc_add_operator
, there will be significant overlap in the information in two different files, with a high likleyhood of them getting out of sync.Other examples where near duplicate doc-strings could get split up between files include.
snap_selected_to_location
/ED_view3d_snap_selected_to_location
view3d_camera_border
/ (ED_view3d_calc_camera_border
/ED_view3d_calc_camera_border_size
).validate_object_select_id
/ED_view3d_select_id_validate
.While the issue of which to document (the static or the public function) already exists with the current convention, when doc-strings are in the implementation it's obvious when the public function is a wrapper for a static function and quick to jump to the static function and see it's doc-string (which is in the same file).
Added subscriber: @Blendify
There will always be developers who don't update things. I've also seen this happen with above-the-implementation documentation, so I don't think this is a particularly strong argument.
This is indeed an issue, but it cannot be solved by having documentation in one place. Until we ditch C (or the language changes drastically), there will always be two places where documentation can be added.
True, although many IDEs show documentation on hovering. Also I think that the need to read the docs while stepping through code is less frequent than the regular need to understand how to use a certain module.
This is an important point. It's not that hard to check though, because as soon as the functionality of some code changes, so should its documentation. If there is no change to that, you know something is wrong; either there is no documentation, which is bad, or it wasn't updated, which is also bad.
I don't quite follow you here. Given that our current guideline is to have doc-strings at the implementation, how are current problems an argument in favour of this? To me it just shows that, no matter where the documentation is located, there will always be lazy/hasty people who don't update it.
Yes, IMO they should be. Even with internal headers, there is the distinction between "interface to the rest of the code" and ""private to only the implementation in this file". IMO it's so much nicer to be able to read through the "this is how you use it" documentation, without having to juggle implementation details and static functions, that it's definitely nicer to have this also for internal code.
I think your example is fine. Until we actually build & publish doxygen documentation, I feel that it's of a lower priority than to have actually understandable, human-readable docs. If it's really a hard choice between
\param
on the interface or the implementation, I'd say use them in the interface documentation.UPDATE: I added my response to other comments here, so that I only have one comment that has everything I want to reply.
The static function doc can simply refer to the public function, IMO. If it really becomes hard to document something well, to me it's an indication that the code structure itself is wrong, and should be redesigned. We should design testable, documentable code, and not avoid high-quality documentation because the code is hard to describe.
To me this is the wrong way around. What a function does, and in which situations it's expected to work, is part of its public interface documentation. The fact that it defers a big part of its code to some internal function, that's an implementation detail. If that static, internal function is at the same abstraction level as the public functions, yet cannot be documented in terms of those public functions, again I see it as a code design issue, and not as a reason to avoid documenting the public interface.
Clean Code by Robert Martin suggests that high-level functions preceed low-level functions. When reading a file top to bottom, it allows you to read the broad design first, and only later get down to the nitty-gritty of the low-level helper functions. To me this makes a lot of sense. However, this high-to-low ordering is uncommon in C, as low-level functions don't require separate declarations if they are defined before they are used. This does present us with considerable issues in terms of understandability of the code, and I think also that this discussion is also related to it. When the high-level internal functions are well documented & seen first, the lower-level functions can be described more succintly, as higher-level concepts have already been explained.
Documentation may include details or descriptions of subtle behavior, if the text documenting this is stored elsewhere it's less obvious when changes to the code end up contradicting statements made in those doc-strings.
This is an observation of what happened in the past, while not exactly the same as C/C++ header comments in 2.4x the Python documentation for e.g. continually got out of sync when the docs were maintained in separate files, since moving the doc-strings directly above the functions this has been much less of a problem.
Not sure what you mean, as long as C++ code uses headers and source files there will be two places that can be documented (unless you mean where implementations are in the header).
Good point.
True, although doc-strings can include all sorts of non-obvious details about arguments which I wouldn't necessarily be able to know without looking at the doc-string if the doc-string needs updating.
The point is I believe maintaining docs in separate files requires some additional discipline (in an area we have not been all that disciplined).
Seems fine, another issue: would we expect doc-strings to be above static function declarations too? (where static functions are forward declared before they are defined within a file)
Perhaps this can be made int a guide-line then (even as part of this proposal), that static functions should refer to public functions docs instead of duplicating them.
There still remains the awkward issue of the static function containing arguments not found in the public function but I don't think there is a good way around this.
Not against this but seems like it could cause a lot of code-churn if applied to the current code-base, so best make that part of a separate proposal (even if it's a guide-line for new code).
I agree that this is something we'll have to keep an eye on.
I was thinking about even more futuristic C++ and having "modules" instead of header+source file separation.
It's my hope that simply checking whether documentation is still correct becomes normal, especially when we move to a situation where documenting the public interface is normal.
Yup, important point. I still think the location of documentation should be primarily chosen for the benefit of the reader of the code, though, and not the writer. To me this is also about having the code-base be welcoming to new developers (or old developers looking into an area of Blender new to them).
Good question. Personally I don't feel particularly strong about this, because those are typically for internal use only. Actually, documenting those at the implementation level could be beneficial; it's probably easier to understand the code when you first read through the high-level implementations, and only then get to the (documented) lower-level helper functions. The documentation for low-level helper functions will be hard to understand without the context of the high-level implementation.
Sounds good to me.
These additional arguments can be documented without any conflict, right?
I agree, better to keep that for a separate proposal.
Having an exception for static functions seems reasonable, especially when these forward declarations may be added based on static function order in the file.
Right, it's just awkward when only some arguments are documented (makes it read as if they're missing).
Although an explicit reference to the public function may be OK in this case.
So the next step to come up with proposed text for https://wiki.blender.org/wiki/Style_Guide/C_Cpp#API_Docs I'd suppose.
Yes, I would envision something like "The first 4 parameters are the same as in #the_public_function".
I think the beginning of the text (about the doxygen) can be kept, up to and including the "Note that this is just the typical paragraph style used in blender with an extra leading '*'. " line.
The code example should change so that it's a function declaration and not a definition.
For the rest of the text, how's this? The emphasis is to aid in scannability of the text, to quickly understand its structure.
As for placement of documentation, follow these guidelines:
When there is overlap between internal and public functions, for example when two public functions actually call an internal function with some additional parameters, the internal function's documentation can refer to the public function. That way documentation doesn't have to be copied between those.
In Summary:
LGTM, worth mentioning some details.
*_internal.h
headers.\param
and\return
) to public doc-stringsI think this only applies when there are two comment blocks (interface & implementation) on the same symbol. Personally I don't see any reason to forbid using
\param
and\return
when documenting an internal function.Added subscriber: @JamesThePlant
Edit the updated proposal addresses this issue.
Ah that was poorly worded, the point remains though, that doubling up on
\param
and\return
in the case there are both public and private doc-strings should be avoided (doxygen reports a warning in this case), although, as noted in a previous comment, I don't think there is much of a need for this for the implementation doc-string.Changed status from 'Confirmed' to: 'Resolved'
Resolved by updating https://wiki.blender.org/wiki/Style_Guide/C_Cpp#API_Docs
This issue was referenced by
c4e041da23
This issue was referenced by
ffc4c126f5
This issue was referenced by
00f3957b8e
This issue was referenced by
d6c3ea9e7a
This issue was referenced by
2545119112
This issue was referenced by
da67a19ed9
This issue was referenced by
db795a4727
This issue was referenced by
7e92717439
This issue was referenced by
93ba5e2375
This issue was referenced by
4f48b2992b
This issue was referenced by
07726ef1b6
This issue was referenced by
2c0ccb0159
This issue was referenced by
a46ff1dd38
This issue was referenced by
e89d42ddff
This issue was referenced by
32b1a13fa1
This issue was referenced by
61776befc3
This issue was referenced by
9e365069af
This issue was referenced by
9f546d6908
This issue was referenced by
7c76bdca1b
This issue was referenced by
8aed5dbcf8
This issue was referenced by
15a428eab0
This issue was referenced by
3647a1e621
This issue was referenced by
cd4a7be5b2
This issue was referenced by
bc01003673
This issue was referenced by
65de17ece4
This issue was referenced by
7f4878ac7f
This issue was referenced by
50f378e5c8
This issue was referenced by
74e57efb2d
This issue was referenced by
d812e46f40
This issue was referenced by
566a458950
This issue was referenced by
3060217d39
This issue was referenced by
63f8d18c0f
This issue was referenced by
bd2b48e98d
This issue was referenced by
744369c114