hdMaya: Adopt the include directive/order according to coding guideline. #379
Conversation
There was a problem hiding this comment.
Looks great to me! Just a few super-tiny nit picks.
Thanks @HamedSabri-adsk!
| @@ -26,7 +26,7 @@ | |||
| #include <maya/MFnLight.h> | |||
| #include <maya/MFnNonExtendedLight.h> | |||
|
|
|||
| #include "dagAdapter.h" | |||
| #include <hdMaya/adapters/dagAdapter.h> | |||
There was a problem hiding this comment.
I think this one would go in its own group below <pxr/imaging/hd/light.h>?
There was a problem hiding this comment.
Thanks you @mattyjams . Please see 7cb7810
| #include <hdMaya/adapters/materialAdapter.h> | ||
| #include <hdMaya/adapters/mayaAttrs.h> | ||
| #include <hdMaya/adapters/tokens.h> | ||
|
|
There was a problem hiding this comment.
Remove this blank line to put all the hdMaya includes in a group together?
There was a problem hiding this comment.
see 7cb7810
| #include <hdMaya/delegates/proxyUsdImagingDelegate.h> | ||
| #include <hdMaya/adapters/shapeAdapter.h> |
There was a problem hiding this comment.
Swap these two so "adapters" is before "delegates"?
There was a problem hiding this comment.
see 7cb7810
| #include <maya/MDGMessage.h> | ||
| #include <maya/MDagPath.h> | ||
| #include <maya/MDagPathArray.h> | ||
| #include <maya/MDGMessage.h> |
There was a problem hiding this comment.
I always waffle on this, whether to order capital letters before lower case, or to sort case-insensitively. :/
There was a problem hiding this comment.
I hear you. I honestly don't have a strong preference. I can change it if you guys prefer it the other way any-times.
There was a problem hiding this comment.
Yeah, I don't have a strong preference either. Just maybe a justification as to why it may have been that way. :)
There was a problem hiding this comment.
I want to go with whatever clang-format does by default for this, since I'd eventually like to start using that to handle header ordering automatically. I wonder if maybe we should just create an initial .clang-format file, which disables everything EXCEPT the header ordering, and then just run that on the whole repo? That way we can at least start having that be standardized, before we come to agreement on all the other nitty-gritty style details.
| #include "primReader.h" | ||
| #include "primReaderRegistry.h" | ||
| #include "mayaUsd/fileio/primReader.h" | ||
| #include "mayaUsd/fileio/primReaderRegistry.h" |
There was a problem hiding this comment.
I think that since, these are installed + available from a PUBLIC include dir, that these should use <>
| @@ -13,15 +13,15 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
| // | |||
| #include "adapter.h" | |||
| #include "adapterDebugCodes.h" | |||
| #include <hdMaya/adapters/adapter.h> | |||
|
|
|||
| #include <pxr/base/tf/type.h> | |||
|
|
|||
| #include <maya/MNodeMessage.h> | |||
There was a problem hiding this comment.
According to the style guidelines, Autodesk + Maya comes before pxr + usd headers. So these two would be flipped.
Of course, I think we're eventually planning on automating this via clang-format, so if you want to leave this be until we formalize our clang-format file, that's fine - we just need to make sure absolute/relative + <>/"" is handled (since clang-format won't change that.)
There's a few other places where maya vs pxr order is flipped, so I won't bother commenting on all of them.
| @@ -17,15 +17,14 @@ | |||
| #define HDMAYA_ADAPTER_H | |||
|
|
|||
| #include <pxr/pxr.h> | |||
|
|
|||
| #include <pxr/usd/sdf/path.h> | |||
|
|
|||
| #include <maya/MMessage.h> | |||
|
|
|||
| #include <vector> | |||
There was a problem hiding this comment.
vector, as a c++ standard lib, should come first.
| #include <maya/MDGMessage.h> | ||
| #include <maya/MDagPath.h> | ||
| #include <maya/MDagPathArray.h> | ||
| #include <maya/MDGMessage.h> |
There was a problem hiding this comment.
I want to go with whatever clang-format does by default for this, since I'd eventually like to start using that to handle header ordering automatically. I wonder if maybe we should just create an initial .clang-format file, which disables everything EXCEPT the header ordering, and then just run that on the whole repo? That way we can at least start having that be standardized, before we come to agreement on all the other nitty-gritty style details.
|
@HamedSabri-adsk: Sure, works for me. I'll try to make some time to look at those other PRs today. Thanks! |
HamedSabri-adsk
deleted the
sabrih/MAYA-103365/header_inclusion_coding_guideline
branch
This PR changes the
include directive/orderin hdMaya according to guideline.