codingGuidelines.md: clarify some details about headers #400
Conversation
doc/codingGuidelines.md
Outdated
| 5. Autodesk + Maya headers | ||
| 6. Pixar + USD headers | ||
| 7. Your project’s headers | ||
| 8. Conditional includes | ||
| 7. All public headers from this project (maya-usd) |
There was a problem hiding this comment.
maya-usd is an example in here? if you are in hdMaya, then this is where you would put your public header files.
There was a problem hiding this comment.
I meant maya-usd, the name of the whole repo. So if you were in hdMaya, then you would put hdMaya and mayaUsd (and mayaUsdUI, etc) headers all in here. I pushed an update which clarifies my intent a bit (replaced "project" with "repository").
This can be a matter for debate - I can think of three options here:
[I'm using your example of being in the hdMaya library, and deciding where to put hdMaya headers (same sub-unit) vs mayaUsd and mayaUsdUIheaders (same repo, different sub-unit).]
-
A. Only headers from same sub-unit (ie, hdMaya) go in 7 Headers from maya-usd repo but other sub-units (ie, mayaUsd + mayaUsdUI) go in 4.
-
B. All headers from the maya-usd repo (ie, hdMaya, mayaUsd, and mayaUsdUI) all go in 7. Only headers from outside of the maya-usd repo go in 4
-
C. We add a new category to separate same sub-unit headers from other sub-units, ie:
- 7. Headers from this repo, but a different sub-unit than current
- 8. Public headers form this sub-unit
Obviously, exact wording would change once we determine exactly what we mean by a "sub-unit".
Personally, I prefer A B because:
- Conceptually, it's simplest + clearest - no need to figure out what is mean by a "sub-unit"
- As opposed to
BA, it aligns with the most-general to most-specific order. InBA, we would have hdMaya headers come before maya headers, which violates the most-general to most-specific principle. - It's easiest to enforce with clang-format - we simply need a single project-level
.clang-formatfile, with a regexp to catch all headers from the maya-usd project (ie,mayaUsd|mayaUsdUI|hdMaya|.... In theory, you could still enforceBA or C with clang-format, but it would become a lot more complicated - you'd need to have a separate clang-format file at each sub-unit, just to override the header ordering rules. And, since I don't think either clang-format or yaml allow for hierarchical overrides or "include" functionality1, we'd have to have complete copies at each level, with just the header ordering changed.
If we don't vote for A B, my next preference would be C We'd be introducing yet another group, but assuming we can get things managed by .clang-format, this doesn't scare me overmuch.
Regardless, if we go with either A or C, we would still need to decide what was meant by a "sub-unit" - personally, I would vote for a broad definition - ie, have only 3 - pxr stuff (ie, in plugin/pxr), al stuff (in plugin/al), and everything else. (For symmetry, if we also wanted to make plugin/adsk it's own sub-unit, and have 4 total, I'd also be fine with that.) That means that, ie, mayaUsd, mayaUsdUI, and hdMaya would always get grouped together, but at least the stuff from the al and pixar plugins would be separated out, and it would keep the number of .clang-format files to a manageable level.
If we try to get more granular, we'll not only have an explosion in the number of .clang-format files, it gets a bit fuzzier about what is a sub-unit. (Do we go by cmake-project? Because there's currently some libraries that don't have a separate cmake-project. Do we go by output files, ie, libraries? If so, then what about utility headers with no associated .cpp?)
Finally - if we go with B, then it's worth noting that even though things like hdMaya and mayaUsd will get lumped together, simply due to alphabetical sorting they will at least be mostly placed next "similar things", ie:
#include <hdMaya/other.h>
#include <hdMaya/thingie.h>
#include <mayaUsd/bar.h>
#include <mayaUsd/foo.h>
#include <mayaUsd/ui/blah.h>
Notes:
- I'd love to be wrong about this, but a quick search of yaml and clang-format didn't turn up any way to do hierarchical overrides or includes...
There was a problem hiding this comment.
My understanding was that we are heading towards option A (based on the discussion we had during TSC call). This is why the mention of maya-usd in the guideline confused me. Let's see what others think.
There was a problem hiding this comment.
Yeah, I'd opt for A as well. I think that's most inline with what we've been discussing.
There was a problem hiding this comment.
Hmm - I'm curious to hear your folk's thoughts for why you prefer A. It seems to me the worst of both worlds:
- A.
👎 Non-ideal header ordering (doesn't preserve most-generic to most-specific)
👎 Difficult to automate + enforce via clang-format - B.
👎 Non-ideal header ordering (no distinction between this lib and other libs within maya-usd)
👍 Easy to automate + enforce via clang-format - C.
👍 Best header ordering (preserves most-generic to most-specific, differentiates within maya-usd)
👎 Difficult to automate + enforce via clang-format
To clarify what I mean about A not having good ordering - @mattyjams, you asked me to add a pxr header, so we could show that it came before the mayaUsd headers. The problem with A is that usually won't be the case - mayaUsd headers will generally come before pxr headers! Here's an example of header ordering with the 3 options:
Note - I'm using 10/20/30/40 so these arbitrary numbers aren't confused with the include-grouping numbers defined in codingGuidelines.md
A
\\\file hdMaya/source.cpp
#include <vector> // 10. most generic
#include <mayaUsd/foo.h> // 30. more specific
#include <pxr/usd/bar.h> // 20. more generic
#include <hdMaya/baz.h> // 40. most specific
B
\\\file hdMaya/source.cpp
#include <vector> // 10. most generic
#include <pxr/usd/bar.h> // 20. more generic
#include <hdMaya/baz.h> // 40. most specific
#include <mayaUsd/foo.h> // 30. more specific
C
\\\file hdMaya/source.cpp
#include <vector> // 10. most generic
#include <pxr/usd/bar.h> // 20. more generic
#include <mayaUsd/foo.h> // 30. more specific
#include <hdMaya/baz.h> // 40. most specific
Note that though B gets the ordering in this case "wrong", the two that are switched are at least within the same group, so at least the ordering of the groups is always correct - as opposed to A, where the group ordering is generally wrong.
There was a problem hiding this comment.
From my perspective, the most important factor is automation. I don't think anyone really wants to spend time reviewing and commenting on include order. So thank you for bringing up IncludeCategories - this was the detail I was missing. And since you have experience with it, how we will separate Related header from All private headers ?
FYI. I will bring this up with our team to see where people are landing.
There was a problem hiding this comment.
I don't actually have experience with clang-format or clang-tidy. I just started doing some reading after looking through all the changes in the header-related PRs. But for the related header vs. all private headers differentiation, the "IncludeIsMainRegex" section immediately after the "IncludeCategories" sounds promising. It feels like between that and carefully crafted regexes, we should be able to get pretty far.
There was a problem hiding this comment.
If we go with B, I think clang-format should be able to handle all our categories. As @mattyjams pointed out, clang-format actually has built-in support for the Related header... and, in fact, we probably won't even need to mess with the IncludeIsMainRegex - by default it picks up include "foo.h" as being the related header for foo.cpp, so I think we're good there.
I don't actually have extensive knowledge of .clang-format, either - Pal first introduced me to it when we was setting up maya-to-hydra, but I really saw it's benefits there.
If we go with A or C - I was thinking about it a bit more, and it WOULD be doable, with some more work - we'd have to have our .clang-format defined as a template, where cmake would fill in the appropriate "sub-unit" header regex everywhere we want that changed. I would still prefer B though.
There was a problem hiding this comment.
@elrond79 Can you make a test to configure clang-format with rules described by option B and run it by a few files?
There was a problem hiding this comment.
I can, but I'm a bit busy at the moment. I will make time to do this sometime next week.
doc/codingGuidelines.md
Outdated
| @@ -156,8 +162,8 @@ All included public header files from outside and inside the project should be ` | |||
|
|
|||
| Private project’s header files should be `#include`'d using the file name when in the same folder. Private headers may live in sub-directories, but they should never be included using "._" or ".._" as part of a relative paths. For example: | |||
There was a problem hiding this comment.
One comment I heard about this line was to make it more explicit about the fact we are using relative paths, i.e. not a full path as above.
There was a problem hiding this comment.
Will make that change...
- make it explicit that associated headers don't use public formatting - make it clear that all headers from this repo (maya-usd) go at end, not just ones from this library within maya-usd - add a new include-order category for private headers - remove some fancy unicode quotes from code blocks
pmolodo
force-pushed
the
pr/codingGuidelines_header_clarification
branch
from
49d83e1
to
0843aca
Compare
|
Clarified the relative-paths-for-private-includes issue you pointed out. As for this going in after those other PRs, that's fine. |
doc/codingGuidelines.md
Outdated
| @@ -148,16 +148,22 @@ In general, macros should be avoided (see [Modern C++](https://docs.google.com/d | |||
| * Comments for users of classes and functions must be written in headers files. Comments in definition files are meant for contributors and maintainers. | |||
|
|
|||
| ### Include directive | |||
| For source files (.cpp) with an associated header file (.h) that resides in the same directory, it should be `#include`'d with double quotes and no path. This formatting should be followed regardless with whether the associated header is public or private. For example: | |||
There was a problem hiding this comment.
Grammar police: "regardless of"?
doc/codingGuidelines.md
Outdated
| All included public header files from outside and inside the project should be `#include`’d using angle brackets. For example: | ||
| ```cpp | ||
| #include <pxr/base/tf/stringUtils.h> | ||
| #include <mayaUsd/nodes/stageData.h> | ||
| ``` | ||
|
|
||
| Private project’s header files should be `#include`'d using the file name when in the same folder. Private headers may live in sub-directories, but they should never be included using "._" or ".._" as part of a relative paths. For example: | ||
| Private project’s header files should be `#include`'d using the quotes, and a relative path. Private headers may live in the same directory or sub-directories, but they should never be included using "._" or ".._" as part of a relative paths. For example: |
There was a problem hiding this comment.
Towards the beginning: "using double quotes" instead of "using the quotes"
Towards the end: "as part of a relative path." instead of "as part of a relative paths."(plural)
doc/codingGuidelines.md
Outdated
| @@ -187,6 +194,8 @@ Headers should be included in the following order, with each section separated b | |||
| #include <mayaUsd/fileio/shading/shadingModeRegistry.h> | |||
There was a problem hiding this comment.
Maybe add a group with a pxr header or two just before the mayaUsd ones for completeness?
There was a problem hiding this comment.
Agreed that we should probably add a pxr header to help clarify... will hold off on doing that until we make a decision here, however.
There was a problem hiding this comment.
Sounds good. I'm on board for option B.
|
Thanks for catching those mistakes @mattyjams - should be fixed now. |
| @@ -148,16 +148,22 @@ In general, macros should be avoided (see [Modern C++](https://docs.google.com/d | |||
| * Comments for users of classes and functions must be written in headers files. Comments in definition files are meant for contributors and maintainers. | |||
|
|
|||
| ### Include directive | |||
| For source files (.cpp) with an associated header file (.h) that resides in the same directory, it should be `#include`'d with double quotes and no path. This formatting should be followed regardless of with whether the associated header is public or private. For example: | |||
There was a problem hiding this comment.
Ah, sorry, this now reads "regardless of with whether" as opposed to "regardless of whether". :)
doc/codingGuidelines.md
Outdated
| @@ -187,6 +194,8 @@ Headers should be included in the following order, with each section separated b | |||
| #include <mayaUsd/fileio/shading/shadingModeRegistry.h> | |||
There was a problem hiding this comment.
Sounds good. I'm on board for option B.
after discussion, we decided that it made more sense to order headers from most-specific (ie, things from this repo) to most generic (ie, system / c++ headers), as this will catch more problems with "bad" headers at compile time. ie, see this post: https://blog.knatten.org/2010/07/01/the-order-of-include-directives-matter/
|
Ok, reversed the header order, as discussed on the forum, to:
...and codified in .clang-format. |
thanks Krystian for catching it!
not just ones from this library within maya-usd
Arose out of some discussion in #392 (and other associated PRs)