-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clang format fixes #842
Clang format fixes #842
Conversation
Hey @pmolodo, thanks for doing this. My only concern is with replacing MAYAUSD_NS_DEF with MAYAUSD_NS. You've expanded out the namespace token, which is the right thing to do for clang-format. However, MAYAUSD_NS is the namespace that is unversioned, while MAYAUSD_VERSIONED_NS is the versioned namespace. Perhaps we can question whether symbols in the maya-usd namespace need to be versioned or not; because maya-usd defines a core library that can be used by more than one plugin I would tend to err on the safe side and version them. Also, MAYAUSD_NS_DEF abstracted away whether we were doing a Doxygen build or not --- and of course we're currently not doing anything of the sort. But again, since maya-usd is defining a core library, it doesn't seem outlandish to think we some day may want to do such a thing. My suggestion would be to keep MAYAUSD_NS_DEF and simply take the namespace token out of it. Hope this makes sense. |
Ah, makes sense. Well... should we consider just making a new macro for the non-namespace-token version? Mostly to preserve backward-compatibility for dependent code already using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We favor the non-namespace-token version because of clang-format and refactoring the code after the change is trivial. Keeping both, IMHO may be confusing for developers.
With the above, I can't find reasons for preserving backward-compatibility.
ab49436
to
3275bd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @pmolodo. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @pmolodo, very appreciated. Just this last nit-pick and I think we're done.
lib/mayaUsd/base/mayaUsd.h.src
Outdated
// Macros to place the MayaUsd symbols in the versioned namespace, which is how | ||
// they will appear in the shared library, e.g. MayaUsd_v1::Class. | ||
|
||
// Deprecated macro (because it causes problems with clang-format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is no longer accurate, right? We should restore the previous comment, perhaps fixing it to change the plural "Macros" to "Macro".
@pmolodo PF failed on all platforms - looks like changes went in that are were using the macro. Please merge the latest changes from dev and fix compilation errors. |
…truct, enum, union)
...because using a macro with no "namespace" caused problems with clang-format
3275bd7
to
322f102
Compare
Fixed the comment and rebased against latest dev. Tested that it compiles. |
A few clang-format related tweaks, in preparation of running on whole repo:
MAYAUSD_NS_DEF
withnamesapce MAYAUSD_NS
(Test - apply clang-format to repo (no intent on merging it yet) #690 (comment))