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: Use the c++ 17 common [[fallthrough]] attribute #4580

Merged

Conversation

hjmjohnson
Copy link
Member

[fallthrough] indicates that the fall through from the previous case label is intentional and should not be diagnosed by a compiler that warns on fall-through (attribute specifier)

Remove complicated #ifdef macro logic to enforce compiler specific behaviors.

PR Checklist

@hjmjohnson hjmjohnson added the type:Style Style changes: no logic impact (indentation, comments, naming) label Apr 12, 2024
@hjmjohnson hjmjohnson self-assigned this Apr 12, 2024
@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module and removed type:Style Style changes: no logic impact (indentation, comments, naming) labels Apr 12, 2024
[[fallthrough]](C++17)	indicates that the fall through from the
previous case label is intentional and should not be diagnosed by a
compiler that warns on fall-through (attribute specifier)

Remove complicated #ifdef macro logic to enforce compiler
specific behaviors.
Copy link
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Thanks, Hans! Interestingly GitHub thinks that [[fallthrough]](C++17) links to https://github.com/InsightSoftwareConsortium/ITK/pull/C++17 😸 Anyway, no problem, approved 👍

@blowekamp
Copy link
Member

This change looks good. How close are we to 5.4? Do we have another RC? Are we planning some kind of new feature freeze soon?

This seems like using a new C++17 feature. How certain are we that all compilers support this feature correctly?

@hjmjohnson
Copy link
Member Author

@blowekamp I do think all compilers that support c++17 do support this. It is not an onerous addition to the language. Many compilers have used similar non-standard methods. c++17 standardized the mechanism.

I have not looked at "all compilers", but the common ones have definitive support for this feature.

@thewtex
Copy link
Member

thewtex commented Apr 12, 2024

How close are we to 5.4? Do we have another RC?

ITK 5.4RC3 was tagged recently, but an issue was discovered immediately -- 5.4RC4 will be tagged and announced with the fix next week.

We should not add major new features until 5.4.0, but this should be fine -- we have had a C++17 requirement for a while, and it is passing all CI.

@blowekamp
Copy link
Member

Yes, this change "should" be fine. Let us get it in for 5.4rc4?

@thewtex thewtex merged commit ffe4c03 into InsightSoftwareConsortium:master Apr 12, 2024
14 checks passed
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 type:Compiler Compiler support or related warnings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants