Skip to content
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 warnings for enum class. #4661

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrei-sandor
Copy link
Contributor

Used with clang

Replaced \class Doxygen annotations with \enum to avoid warnings

@github-actions github-actions bot added type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module labels May 13, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing a pull request! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.

@seanm
Copy link
Contributor

seanm commented May 13, 2024

@dzenanz @hjmjohnson @N-Dekker : @andrei-sandor is a student working with me this summer, and he's going to take a stab at fixing some warnings in ITK with the goal of being able to enable a few more warning flags on our nightly submissions to cdash.

@N-Dekker
Copy link
Contributor

N-Dekker commented May 14, 2024

Thanks @andrei-sandor ! Cool!

I'm interested to hear if these changes will affect the Doxygen output, for example at https://itk.org/Doxygen/html/classitk_1_1CommonEnums.html

The proposed changes look fine to me, but they caused some (undeserved) KWStyle errors. Looking at https://open.cdash.org/tests/1552684315

/home/vsts/work/1/s/Modules/Core/Common/include/itkCommonEnums.h:49: error: comment doesn't have \class
/home/vsts/work/1/s/Modules/Core/Common/include/itkCommonEnums.h:76: error: comment doesn't have \class
/home/vsts/work/1/s/Modules/Core/Common/include/itkCommonEnums.h:100: error: comment doesn't have \class

Probably coming from https://github.com/Kitware/KWStyle/blob/6971e89ec4c763601edf1d0ea0d6f71d42fbca5d/kwsCheckComments.cxx#L203

Do you (or anyone) know how to address those as well?

@dzenanz
Copy link
Member

dzenanz commented May 14, 2024

Maybe this condition needs to be updated to consider enum in addition to class? Or we just need to update style overwrite rules to add appropriate exceptions - probably Comments or NameOfClass from this list.

Comment on lines -135 to 138
* \class CellGeometry
* \enum CellGeometry
* \ingroup ITKCommon
* Enums used to specify cell type */
enum class CellGeometry : uint8_t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I think Doxygen should be able to automatically "defer" that CellGeometry is an enum, just by looking at the code. So then the whole line saying * \... CellGeometry might simply be removed. But that may be beyond the scope of this pull request, so for now replacing \class with \enum looks fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. We could do this instead. But it would be a massive change touching many files.

Is there some reason ITK is explicitly using \class? Did someone think it was required? Is it a stylistic preference to be explicit? Or...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply allowing doxygen to infer that the comments are attached to the enum below (as @N-Dekker suggested) seems best to me, as it will also eliminate needing to mess with KWStyle. What do you think @dzenanz @thewtex ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works, that's fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing the \enum, the following warning is produced Modules/Core/Common/include/itkFloatingPointExceptions.h:37: error: comment doesn't have \class. What are the possible options? Should a modification be done like here style overwrite rules? Or maybe here this condition (adding an enum might be more complex)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants