Skip to content

Conversation

@seanm
Copy link
Contributor

@seanm seanm commented Jun 19, 2025

No description provided.

@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:Core Issues affecting the Core module labels Jun 19, 2025
@seanm seanm changed the title COMP: suppress Clang -Wduplicate-enum WIP: COMP: suppress Clang -Wduplicate-enum Jun 19, 2025
@seanm seanm force-pushed the Wduplicate-enum branch from b62c1d5 to d8632ba Compare June 19, 2025 20:00
@github-actions github-actions bot removed the type:Compiler Compiler support or related warnings label Jun 19, 2025
@seanm seanm changed the title WIP: COMP: suppress Clang -Wduplicate-enum COMP: suppress Clang -Wduplicate-enum Jun 19, 2025
@dzenanz
Copy link
Member

dzenanz commented Jun 19, 2025

Interesting style change by clang-format.

@seanm
Copy link
Contributor Author

seanm commented Jun 19, 2025

Interesting style change by clang-format.

Mmmm, which style change? I added more #if branches, so things got reindented...

@dzenanz
Copy link
Member

dzenanz commented Jun 19, 2025

I meant what clang-format wants by its complaint in pre-commit check.

@seanm
Copy link
Contributor Author

seanm commented Jun 19, 2025

I meant what clang-format wants by its complaint in pre-commit check.

Oh! fugly. :) Odd, because I ran clang-format -i locally before committing. I have version 20.1.6 though, maybe different.

@N-Dekker
Copy link
Contributor

@seanm Thank you very much for addressing these warnings! I wasn't aware of them, when I proposed those enum's (PR #5410).

By the way, it appears that an enumerator like INT8 = CHAR would pass without a warning, even without your pull request:

However, something like INT32 = std::is_same_v<int32_t, long> ? LONG : INT does indeed trigger that warning:

INT32 = std::is_same_v<int32_t, long> ? LONG : INT,

As I observed at https://godbolt.org/z/bPTzc68YK using clang 20.1.0 But unfortunately the std::is_same_v is necessary here anyway 🤷 So disabling them appears to be the best solution 👍

Comment on lines +130 to +134
# if __has_warning("-Wduplicate-enum")
# define ITK_CLANG_SUPPRESS_Wduplicate_enum ITK_PRAGMA(clang diagnostic ignored "-Wduplicate-enum")
# else
# define ITK_CLANG_SUPPRESS_Wduplicate_enum
# endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the if __has_warning is necessary to avoid another warning (warning: unknown warning group '-W...', ignored [-Wunknown-warning-option]). Unfortunately it's a bit cumbersome to have 5 lines of boilerplate code to define such a macro (ITK_CLANG_SUPPRESS_Wduplicate_enum), for just one warning. Would it be possible to make macro definitions like this more concise? And would it be possible to just specify the string "-Wduplicate-enum" once, instead of twice? I'm just asking. 🤷 No show stopper.

Also use __has_warning to guard againist trying to disable warnings that are unknown (ex: in older Clangs).
@seanm seanm force-pushed the Wduplicate-enum branch from d8632ba to f6969f1 Compare June 20, 2025 10:21
@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:Examples Demonstration of the use of classes labels Jun 20, 2025
@seanm
Copy link
Contributor Author

seanm commented Jun 20, 2025

I meant what clang-format wants by its complaint in pre-commit check.

maybe I've fixed it with my change to the clang-format config file...

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 @seanm

If you ever find a way to define macro's like ITK_CLANG_SUPPRESS_Wduplicate_enum in a more concise manner, please let us know. Approved anyway 👍

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Thanks @seanm 🙏

@thewtex thewtex merged commit a45fb7f into InsightSoftwareConsortium:master Jun 20, 2025
16 checks passed
@N-Dekker
Copy link
Contributor

By the way, it appears that an enumerator like INT8 = CHAR would pass without a warning, even without your pull request

To further elaborate:

  enum class MyEnum
  {
    UNKNOWN,
    CHAR,
    INT8 = CHAR,  // Intended duplicate value, no problem, no warning!
    WONDERFUL = 42,  // OK, new enum value
    DUPLICATE_ZERO = 0  // <== Warning! Duplicate value!
  };

See https://godbolt.org/z/78r883836

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:Examples Demonstration of the use of classes type:Compiler Compiler support or related warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants