Re: [lmi] Stifling "unused" warnings

[Greg Chicares]

On 10/27/21 11:51 PM, Vadim Zeitlin wrote:
> On Wed, 27 Oct 2021 22:33:32 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
> GC> Since commit bc7259fb, the question is whether clang
> GC> gives zero warnings with origin/master, but gives
> GC> exactly the two warnings expected with the patch below.
> 
>  clang-12 gives only one of the expected warnings:
> 
> miscellany_test.cpp:470:9: error: private field 'unused_' is not used 
> [-Werror,-Wunused-private-field]
>     int unused_;
>         ^

Thanks. This suggests that class partly_unused is a valid distillation of

  SequenceParser::SequenceParser
  ...
    // Suppress clang '-Wunused-private-field' warnings:
    stifle_unused_warning(inforce_duration_);
    stifle_unused_warning(effective_year_);

in 'input_sequence_parser.cpp'.

However, this test in 'miscellany_test.cpp' still doesn't behave
as hoped:

    // Variable initialized and later used...
    int volatile d {4};
//  stifle_unused_warning(d); // [see below]
    // ...but last value assigned...
    for(int i = 0; i < 7; ++i)
        {
        d = static_cast<int>(std::clock());
        }
    // ...is not subsequently used. In this case, for clang at least,
    // it is necessary to stifle the warning here:
    stifle_unused_warning(d);
    // rather than in the commented-out location above. See:
    //   https://lists.nongnu.org/archive/html/lmi/2021-04/msg00058.html

The hope was that it would reproduce the problem discussed here:
  https://lists.nongnu.org/archive/html/lmi/2021-04/msg00058.html
and fixed in commit 0380e928ce608. The motivation is to make the
unit tests encompass all the use cases in lmi, especially those
that exhibit subtleties that we've needed to work through, so that
the unit-test module is a full collection of self-contained simple
test cases, and we can therefore test with experimental alternative
stifle_unused_warning() implementations just by building the unit
tests, without having to build all of lmi with various options and
compilers.

This particular unit test attempts to distill this situation:

void mete0()
{
    double volatile x;
    for(int j = 0; j < 100000; ++j)
        {
        x = i_upper_12_over_12_from_i_naive<double>()(0.04);
        x = i_from_i_upper_12_over_12_naive<double>()(0.04);
        x = d_upper_12_from_i_naive        <double>()(0.04);
        x = net_i_from_gross_naive<double,365>()(0.04, 0.007, 0.003);
        }
    stifle_unused_warning(x);
}

where we had moved the "stifle_unused_warning(x);" line from its
original location (right below the "double volatile x;" line)
because, as the commit message explains:

    Otherwise this attempt to suppress gcc -Wunused-but-set-variable warning
    results in -Wuninitialized-const-reference from clang because the
    variable was indeed not initialized yet when it was referenced from
    inside stifle_warning_for_unused_value().

Now I think I see my mistake. In reality, the problem is simple--the
reason why the original code failed seems obvious if we isolate it:

     double volatile x;
     stifle_warning_for_unused_value(x);

I.e., an uninitialized variable has no unused value because it has no value.
Somehow I had thought that the problem was that the first 399,999 values
assigned to 'x' were considered "used", but the 400,000th was "unused";
but actually the value of 'x' is never referenced, and 'x' is a write-only
sink.

> I don't fully understand what's going on here, but I just can't get any
> -Wunused-variable for "d" with clang 12 unless I remove the assignment to
> it inside the loop, even removing volatile doesn't help. I.e. somehow
> initializing a variable and not using it provokes this warning, but
> initializing a variable and then assigning to it suppresses it.

Thus, as an alternative to commit 0380e928ce:

     double volatile x;
-    stifle_warning_for_unused_value(x);
     for(int j = 0; j < 100000; ++j)
...
+    stifle_warning_for_unused_value(x);

this would probably have sufficed:

-    double volatile x;
+    double volatile x {};
     stifle_warning_for_unused_value(x);

>  FWIW gcc 11 does give a warning with this patch
> 
> miscellany_test.cpp: In function ‘void test_stifle_unused_warning()’:
> miscellany_test.cpp:492:18: error: variable ‘d’ set but not used 
> [-Werror=unused-but-set-variable]
>   492 |     int volatile d {4};
>       |                  ^
> 
>  The real problem, however, is that it also gives a warning without this
> patch, resulting in a new CI build failure:
> 
> miscellany_test.cpp: In function ‘void test_stifle_unused_warning()’:
> miscellany_test.cpp:477:26: error: ‘a’ may be used uninitialized 
> [-Werror=maybe-uninitialized]
>   477 |     stifle_unused_warning(a);
>       |     ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from miscellany_test.cpp:24:
> miscellany.hpp:296:16: note: by argument 1 of type ‘const int&’ to ‘constexpr 
> void stifle_unused_warning(const T&) [with T = int]’ declared here
>   296 | constexpr void stifle_unused_warning(T const&)
>       |                ^~~~~~~~~~~~~~~~~~~~~
> miscellany_test.cpp:476:9: note: ‘a’ declared here
>   476 |     int a;
>       |         ^

That seems to be a valid and reasonable warning, which we should
endeavor to avoid as we rethink the stifle_unused_warning()
implementation.

>  So we now have the same problem as we used to have for clang (but somehow
> don't any more) for gcc 11. Removing the const from the function signature
> does fix it, please see my stifle-unused-non-const branch on GitHub which
> passed CI, but I wonder if we shouldn't actually take a "T&&" here to allow
> passing r-value references too, to allow using things like
> 
>       stifle_unused_warning(a + b);
> 
> for example, what do you think?

The original suggestion here:
  https://herbsutter.com/2009/10/18/mailbag-shutting-up-compiler-warnings/
was apparently
  template<class T> void ignore(T&) { }
but was later changed to
  template<class T> void ignore(T const&) { }
because
| The explicit const lets you also handle temporaries.
| I’ll update the post to add the const.

That's an interesting use case, so I added tests for it in a commit
that I just pushed. Although interesting, I'm not convinced that
it's actually useful.

I also pushed a commit that does this:

-    // Variable initialized and later used...
-    int volatile d {4};
+    // Variable uninitialized at first...
+    int volatile d;
 //  stifle_unused_warning(d); // [see below]
-    // ...but last value assigned...
+    // ...though set later, but last value assigned...

which might remove some of the confusion discussed above.

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

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

- if variadic templates are not supported:
    ignore_unused(T1&)
    ignore_unused(T1 const&)
    ignore_unused()
  along with additional overloads for more than one argument or typename

- if variadic templates are supported:
    ignore_unused(T&& ...)

- if variadic templates are supported, but rvalue references are not:
    ignore_unused(T const& ...)

This design seems asymmetric at best and discordant at worst.
I don't think it's necessary to accommodate multiple arguments.
I'd be inclined to try a single (not overloaded) implementation
using a "universal reference", i.e.:

  template<typename T> constexpr void stifle_unused_warning(T&&) {}

and I'm so hopeful it'll address all use cases that I've gone ahead
and committed it--pushed just now. What do you think? Does the unit
test encompass all of lmi's use cases as well as any others that we
or other people have thought of? And does the universal-reference
implementation address all of this cases in the best way?

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