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

Silence msvc warning about an unused named parameter #2289

Merged

Conversation

DanielaE
Copy link
Contributor

Warning C4100 may cause compile failures under strict warning regimes.

Warning C4100 may cause compile failures under strict warning regimes.
@@ -2446,6 +2446,7 @@ constexpr int get_arg_index_by_name(basic_string_view<Char> name) {
}
if constexpr (sizeof...(Args) > 0)
return get_arg_index_by_name<N + 1, Args...>(name);
(void)name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a bug in MSVC because name is used in the line above. I recommend reporting to Microsoft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so: if the control flow in an instantiation of

template <int N, typename T, typename... Args, typename Char>
constexpr int get_arg_index_by_name(basic_string_view<Char> name)

reaches line 2449, then both if constexpr statements before were evaluated as false and their first (in this case: only) substatements become so called discarded statements [stmt.if]/2 which leave no use of name in that template specialization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was also discussed on Microsoft Developer Community, and resulted in joining the other compilers (GCC and Clang, at least) for these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for both links!

Regarding the supposed GCC bug: the original poster gave a wrong description of the nature of the problem. He was missing the difference between a runtime if and a compile-time if constexpr as layed out in the standard. And "This is a valid program which should not produce a warning" misses the point of -Wunused-but-set-parameter explicitly requesting a warning.

This is exactly what the last poster on MS DevCon is rightfully complaining about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've referenced this PR on DevCon. According to my knowledge of the release schedule this will certainly not make it for 16.10 which will probably be around for a long time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, whatever is preferred here. It's just the same as a few lines later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for investigating and I'm glad that this nonsense has already been reported. Merged this as a workaround for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

"This is a valid program which should not produce a warning" misses the point of -Wunused-but-set-parameter explicitly requesting a warning.

I disagree, using a parameter in one branch of if constexpr is not conceptually different from using it in one branch of if (that may be optimized away). It's great that it was fixed in GCC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "misses" is referring to the blanket statement. A valid program may have many good reasons to issue warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that is true.

@vitaut vitaut merged commit 6469b90 into fmtlib:master May 16, 2021
@DanielaE DanielaE deleted the feature/silence-unused-parameter-warning branch May 16, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants