| Message ID | 20240828093804.1682841-1-jason@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: unused local declarations | expand |
On Wed, 28 Aug 2024 at 10:38, Jason Merrill <jason@redhat.com> wrote: > > Tested x86_64-pc-linux-gnu. OK for trunk, or were these supposed to be used? I think the _RefT typedef was used in an earlier version of the code (possibly a much earlier version that was never even pushed). It can be removed. I think the __ok variable exists to force constant evaluation of __check_dynamic_spec_types, although I think that could be done this way instead: if (is_constant_evaluated()) __check_dynamic_spec_types(); I'll take care of both changes today, so please don't bother pushing this. > > -- 8< -- > > The __ok variable and _RefT typedef aren't used in the rest of their blocks. > Should they be? > > libstdc++-v3/ChangeLog: > > * include/std/format (check_dynamic_spec): Add [[maybe_unused]]. > * include/std/ranges (to): Likewise. > --- > libstdc++-v3/include/std/format | 1 + > libstdc++-v3/include/std/ranges | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format > index 3280dadfb90..376169018ba 100644 > --- a/libstdc++-v3/include/std/format > +++ b/libstdc++-v3/include/std/format > @@ -4323,6 +4323,7 @@ namespace __format > constexpr void > basic_format_parse_context<_CharT>::check_dynamic_spec(size_t __id) noexcept > { > + [[maybe_unused]] > constexpr bool __ok = __check_dynamic_spec_types<_Ts...>(); > > if consteval { > diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges > index b7c7aa36ddc..008461e556b 100644 > --- a/libstdc++-v3/include/std/ranges > +++ b/libstdc++-v3/include/std/ranges > @@ -9327,7 +9327,7 @@ namespace __detail > std::forward<_Args>(__args)...); > else > { > - using _RefT = range_reference_t<_Rg>; > + using _RefT [[maybe_unused]] = range_reference_t<_Rg>; > static_assert(constructible_from<_Cont, _Args...>); > _Cont __c(std::forward<_Args>(__args)...); > if constexpr (sized_range<_Rg> > > base-commit: 48032f28ad4bc5e810c303229bcaa223a0c4d09f > -- > 2.46.0 >
On Wed, 28 Aug 2024 at 10:45, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Wed, 28 Aug 2024 at 10:38, Jason Merrill <jason@redhat.com> wrote: > > > > Tested x86_64-pc-linux-gnu. OK for trunk, or were these supposed to be used? > > I think the _RefT typedef was used in an earlier version of the code > (possibly a much earlier version that was never even pushed). It can > be removed. It should have been removed in r14-6199-g45630fbcf7875b. > > I think the __ok variable exists to force constant evaluation of > __check_dynamic_spec_types, although I think that could be done this > way instead: > > if (is_constant_evaluated()) > __check_dynamic_spec_types(); That's not right because it needs to be called for non-constexpr evaluations too. I tried simply making it return void and then calling it like this: diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format index 3280dadfb90..450d65dc80d 100644 --- a/libstdc++-v3/include/std/format +++ b/libstdc++-v3/include/std/format @@ -305,7 +305,7 @@ namespace __format private: // Check the Mandates: condition for check_dynamic_spec<Ts...>(n) template<typename... _Ts> - static consteval bool + static consteval void __check_dynamic_spec_types() { if constexpr (sizeof...(_Ts)) @@ -334,7 +334,6 @@ namespace __format if (__sum != sizeof...(_Ts)) __invalid_dynamic_spec("disallowed template argument type"); } - return true; } // This must not be constexpr. @@ -4323,7 +4322,7 @@ namespace __format constexpr void basic_format_parse_context<_CharT>::check_dynamic_spec(size_t __id) noexcept { - constexpr bool __ok = __check_dynamic_spec_types<_Ts...>(); + __check_dynamic_spec_types<_Ts...>(); if consteval { if (__id >= _M_num_args) But now the std/format/parse_ctx_neg.cc test fails. I think the code actually works as intended, but the quality of the diagnostic suffers due to the change above. The errors no longer point to the line where the immediately-escalating expression is originally called from, and so the { dg-error "here" } lines in the test no longer match. So I think keeping the constexpr local variable in the function is preferable, so marking it [[maybe_unused]] is the right fix.
diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format index 3280dadfb90..376169018ba 100644 --- a/libstdc++-v3/include/std/format +++ b/libstdc++-v3/include/std/format @@ -4323,6 +4323,7 @@ namespace __format constexpr void basic_format_parse_context<_CharT>::check_dynamic_spec(size_t __id) noexcept { + [[maybe_unused]] constexpr bool __ok = __check_dynamic_spec_types<_Ts...>(); if consteval { diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges index b7c7aa36ddc..008461e556b 100644 --- a/libstdc++-v3/include/std/ranges +++ b/libstdc++-v3/include/std/ranges @@ -9327,7 +9327,7 @@ namespace __detail std::forward<_Args>(__args)...); else { - using _RefT = range_reference_t<_Rg>; + using _RefT [[maybe_unused]] = range_reference_t<_Rg>; static_assert(constructible_from<_Cont, _Args...>); _Cont __c(std::forward<_Args>(__args)...); if constexpr (sized_range<_Rg>