-
-
Notifications
You must be signed in to change notification settings - Fork 659
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
COMP: Fix the WDocumentation \struct warning for template struct #4662
COMP: Fix the WDocumentation \struct warning for template struct #4662
Conversation
For this bug llvm/llvm-project#92023 related to clang, the solution is to remove the \struc Doxygen comment. Should this modification be performed each time that we have a template/struct combination? |
If removing the |
@dzenanz @jhlegarreta if no one else objects, we can make this change everywhere. It seems to be the only way to work around that clang -Wdocumentation bug, and it would be nice to get to zero -Wdocumentation warnings. |
/** | ||
* \brief Templated class to produce a unique type "true" and "false". |
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.
You might as well merge those two lines of comment (line 57 and 58), once \struct BooleanDispatch
is removed. As in:
/** \brief Templated class to produce a unique type "true" and "false".
Just a suggestion!
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.
Yes, I applied the suggestion. Thank you!
@seanm I am not sure what is the right way to go, but Doxygen does have a So the underlying problem may be another one. |
@jhlegarreta the issue is that clang's -Wdocumentation warning is triggered by this construct: /** \struct BooleanDispatch
* \brief Templated class to produce a unique type "true" and "false".
*
* BooleanDispatch is a templated class that produce a unique type for
* for "true" and for "false". These types may be used to decide which
* version of an overloaded function to call.
*/
template <bool>
struct BooleanDispatch
{}; It warns:
I think it's confused by the intermediate presence of the So far, this is the only false positive we've seen from -Wdocumentation, which has caught other nice things, so I'd like to enable it on CI. |
I will not oppose to it, but wondering if there is a way to tell clang -Wdocumentation to skip this particular line. And yes, enabling it in the CI would be great. Thanks Sean. |
It could be wrapped with |
OK, as said, will not oppose. It's been a while since I do not have enough bandwidth to contribute/review as I'd like to, so go ahead with all changes as required without waiting for me. |
b9c1667
to
b249b64
Compare
Remove \struct Doxygen keyword from template struct Fix the others WDocumentation \struct warnings for template struct Also, changed that \brief starts on /**
b249b64
to
59c5950
Compare
@dzenanz how do you kick a bot again? |
/azp.run ITK.macOS.Arm64 |
/azp run ITK.macOS.Arm64 |
I don't think either of those "commands" worked. I kicked it off via GUI. |
Where in the GUI can one do this? Or can you because you have admin privileges? Anyway, I'm trying to tell what's failing on that macOS bot. I see one test "itkFFTConvolutionImageFilterStreamingTest" failing, is that the issue? Is that test known-flaky? This PR changes only comments, so I don't get why the test would fail... |
@thewtex might comment on the failing test flaky-ness. |
Ah, thanks for pointing out where that button is! |
I am not familiar with the reported flakiness of itkFFTConvolutionImageFilterStreamingTest. |
Hmm, well, re-runnning it for the 3rd time now has the CI green. |
This looks ready to merge to me. |
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.
👍
Remove \struct Doxygen keyword from template struct