Re: [lmi] Stifling "unused" warnings

[Vadim Zeitlin]

On Thu, 28 Oct 2021 16:16:05 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> This particular unit test attempts to distill this situation:
GC> 
GC> void mete0()
GC> {
GC>     double volatile x;
GC>     for(int j = 0; j < 100000; ++j)
GC>         {
GC>         x = i_upper_12_over_12_from_i_naive<double>()(0.04);
GC>         x = i_from_i_upper_12_over_12_naive<double>()(0.04);
GC>         x = d_upper_12_from_i_naive        <double>()(0.04);
GC>         x = net_i_from_gross_naive<double,365>()(0.04, 0.007, 0.003);
GC>         }
GC>     stifle_unused_warning(x);
GC> }
GC> 
GC> where we had moved the "stifle_unused_warning(x);" line from its
GC> original location (right below the "double volatile x;" line)
GC> because, as the commit message explains:
GC> 
GC>     Otherwise this attempt to suppress gcc -Wunused-but-set-variable warning
GC>     results in -Wuninitialized-const-reference from clang because the
GC>     variable was indeed not initialized yet when it was referenced from
GC>     inside stifle_warning_for_unused_value().
GC> 
GC> Now I think I see my mistake. In reality, the problem is simple--the
GC> reason why the original code failed seems obvious if we isolate it:
GC> 
GC>      double volatile x;
GC>      stifle_warning_for_unused_value(x);
GC> 
GC> I.e., an uninitialized variable has no unused value because it has no value.
GC> Somehow I had thought that the problem was that the first 399,999 values
GC> assigned to 'x' were considered "used", but the 400,000th was "unused";
GC> but actually the value of 'x' is never referenced, and 'x' is a write-only
GC> sink.

 Yes, exactly.

GC> > I don't fully understand what's going on here, but I just can't get any
GC> > -Wunused-variable for "d" with clang 12 unless I remove the assignment to
GC> > it inside the loop, even removing volatile doesn't help. I.e. somehow
GC> > initializing a variable and not using it provokes this warning, but
GC> > initializing a variable and then assigning to it suppresses it.
GC> 
GC> Thus, as an alternative to commit 0380e928ce:
GC> 
GC>      double volatile x;
GC> -    stifle_warning_for_unused_value(x);
GC>      for(int j = 0; j < 100000; ++j)
GC> ...
GC> +    stifle_warning_for_unused_value(x);
GC> 
GC> this would probably have sufficed:
GC> 
GC> -    double volatile x;
GC> +    double volatile x {};
GC>      stifle_warning_for_unused_value(x);

 Yes, it would, but while initializing variables is in general a good idea,
it seems a tiny bit strange to do it only to avoid a warning which itself
occurs only due to a workaround for another warning (i.e. it's not even a
naturally occurring warning, but a second order effect).

GC> The original suggestion here:
GC>   https://herbsutter.com/2009/10/18/mailbag-shutting-up-compiler-warnings/
GC> was apparently
GC>   template<class T> void ignore(T&) { }
GC> but was later changed to
GC>   template<class T> void ignore(T const&) { }
GC> because
GC> | The explicit const lets you also handle temporaries.
GC> | I’ll update the post to add the const.
GC> 
GC> That's an interesting use case, so I added tests for it in a commit
GC> that I just pushed. Although interesting, I'm not convinced that
GC> it's actually useful.

 Me neither, which is why I asked. IMO you should take "T&" in this
function if you want to actively disallow passing temporaries to it.
Otherwise it should take either "T const&", if you accept to have to
initialize all the variables used with it, or "T&&" if you don't want to
bother with it.

GC> Assuming this unit test now covers every use case actually in lmi,
GC> as well as the ignore-a-temporary idea above, we can discuss
GC> implementations. Here you're set up to use gcc++-11 and clang,
GC> whereas I'm not, so it might be more efficient to ask you to
GC> try each implementation and reply with your findings.

 The latest stifle_unused_warning() version works fine with both gcc and
clang and the tests seem representative to me, so AFAICS this problem is
completely solved now.

GC> The candidate implementations we've discussed include:
GC>   (1) template<typename T> constexpr void stifle_unused_warning(T const&) {}
GC>   (2) template<typename T> constexpr void stifle_unused_warning(T&) {}
GC>   (3) template<typename T> constexpr void stifle_unused_warning(T&&) {}
GC> I also looked at boost's:
GC>   
https://github.com/boostorg/core/blob/12f5f51427fbc9d27f56e5de002b0008fea420c9/include/boost/core/ignore_unused.hpp
GC> which seems curious.
[...]

 Boost tries to support even more C++ compilers than wxWidgets does. I
don't envy them their task, and I think this can be valuable, but you can
definitely do better if you only support (reasonably) standard-compliant
C++20 compilers as we do.

GC> I don't think it's necessary to accommodate multiple arguments.

 Yes, I really don't know why did they do it. I tried to think about some
scenarios involving eldritch horrors (a.k.a. C preprocessor macros), but
even then I couldn't come up with a realistic example where this would be
needed.

GC> I'd be inclined to try a single (not overloaded) implementation
GC> using a "universal reference", i.e.:
GC> 
GC>   template<typename T> constexpr void stifle_unused_warning(T&&) {}
GC> 
GC> and I'm so hopeful it'll address all use cases that I've gone ahead
GC> and committed it--pushed just now. What do you think?

 I think it's perfectly fine if you want to support passing temporaries to
this function (and I don't see any reason to forbid this).

GC> Does the unit test encompass all of lmi's use cases as well as any
GC> others that we or other people have thought of? And does the
GC> universal-reference implementation address all of this cases in the
GC> best way?

 AFAICS, yes.

GC> And can the 'd' test be simplified to
GC>     int volatile d;
GC>     stifle_unused_warning(d);
GC> in light of the discussion above? (If so, then it becomes a trivial
GC> echo of the 'a' case.)

 Yes, I think it's the same test written in a more complicated way,
basically.

 Thanks for fixing this!
VZ

pgpFMfVouHClZ.pgp
Description: PGP signature