Re: [lmi] PATCH: simple clang -Wuninitialized-const-reference fix

[Greg Chicares]

On 4/24/21 11:18 PM, Vadim Zeitlin wrote:
>  The following simple patch:
> 
> ---------------------------------- >8 --------------------------------------
> diff --git a/miscellany.hpp b/miscellany.hpp
> index b72439f17..65485574a 100644
> --- a/miscellany.hpp
> +++ b/miscellany.hpp
> @@ -243,7 +243,7 @@ inline void stifle_warning_for_unused_variable(T const&)
>  /// for volatile types.
> 
>  template<typename T>
> -inline void stifle_warning_for_unused_value(T const& t)
> +inline void stifle_warning_for_unused_value(T& t)
>  {
>      (void)&t;
>  }
> ---------------------------------- >8 --------------------------------------
> 
> also available as the only commit in the branch "clang-12-uninit-const-ref"
> of vadz/lmi on GitHub or at https://github.com/let-me-illustrate/lmi/pull/181
> fixes the following error
> 
> ---------------------------------- >8 --------------------------------------
> math_functions_test.cpp:203:37: error: variable 'x' is uninitialized when 
> passed as a const reference argument here 
> [-Werror,-Wuninitialized-const-reference]
>     stifle_warning_for_unused_value(x);
>                                     ^
> ---------------------------------- >8 --------------------------------------
> 
> (and 5 more identical warnings for the other occurrences of the same
> function in this file) when building lmi with clang 12 under Linux.
> 
>  It's a bit annoying to have to fix a warning which only happens in the
> result of trying to fix another warning, but I don't see any better
> solution and this one doesn't seem that bad.
> 
>  Could you please apply it or let me know if you have any better ideas?

I feel uneasy about this change, for two reasons. First, before
this patch, this file contains two closely-related functions:
  template<typename T> inline void stifle_warning_for_unused_variable(T const&)
  template<typename T> inline void stifle_warning_for_unused_value(T const& t)
                                                               ^^^ difference
and it's unsettling to remove 'const' from one but not the other.

Second, ignoring the implementation of this function template, what's
the most general way to specify its parameter? In C++98 at least,
  template<typename T> void f(T const& t)
seemed to be the answer. Rationale:
 - don't pass by value, just in case copying is expensive
 - prefer "const&" to "&" because the parameter won't be modified

If we were proposing this for standardization, hence subjecting it
to criticism from every angle, what would the best experts tell us
that the parameter type ideally ought to be? Wouldn't the advice
be to do as std::max() does? In C++20, that is:

  template<class T> constexpr const T& max(const T& a, const T& b);

so why should we not just add 'constexpr':

  template<typename T> inline void constexpr stifle_warning_for_unused_value(T 
const& t)

but keep 'const'? We aren't going to modify the argument, so why
should we not guarantee that by writing 'const'?

If this were a proposal for standardization, we couldn't simply
state that our implementation in fact does not modify its argument,
because even if we suggest an implementation in a note, only the
function signature would be normative.

At first I wondered whether the answer to this general question
might have changed since C++98 (e.g., use a universal reference?),
but if that were true, they would have changed std::max().

It seemed surprising that I couldn't find much of direct relevance
by searching the web, but maybe the answer is considered obvious.
This is the closest thing I found:

https://stackoverflow.com/questions/4876913/template-pass-by-value-or-const-reference-or
| template<class T> void f(T x) {…}
| template<class T> void f(T const& x) {…}
| I guess that the second option can be more optimal as it explicitly avoids
| a copy, but I suspect that it can also fail for some specific types T
| (eg functors?). So, when should use the first option, and when to use the
| second?
[...answer...]
| Pass by reference-to-const is the only passing mechanism that "never" fails.
| It does not pose any requirements on T, it accepts both lvalues and rvalues
| as arguments, and it allows implicit conversions.

and it does seem to be a legitimate reason for using 'const&' here.

With those objections in mind, I took a step back and reconsidered:
what warning are we receiving, and what warning did we want to avoid?
We have

    double volatile x;
    stifle_warning_for_unused_value(x);
    // ...then assign some value to 'x'

and we don't want the compiler to scold us for not "using" the
last value we assign to 'x'. But now clang is warning us about
something different: AIUI, it's saying that we haven't yet
assigned any value to 'x' at the point where we "stifle" a
(future) warning that we'll (later) assign a value that we
won't ever actually use. Thus:

    double volatile x;
    stifle_warning_for_unused_value(x);
// Here, clang warns that we haven't yet assigned a value
// to 'x', regardless of whether a value that we'll assign
// later will eventually be used or not.
    // ...then assign some value to 'x'
    // return without using the value we assigned
// Here, if we didn't call stifle_warning_for_unused_value(),
// even older versions of gcc would warn that we didn't use
// the value that we did assign.

Thus, the defect isn't in stifle_warning_for_unused_value()
at all; we'd get the same warning for any function foo():
    double volatile x;
    foo(x);
and I think the right thing to do here is:

    double volatile x;
-   stifle_warning_for_unused_value(x);
    // ...then assign some value to 'x'
+   stifle_warning_for_unused_value(x);

IOW, stifle the unused-value warning only in a context
where a value has already been assigned. What do you think?

In any case, should we add 'constexpr' to both "stifle"
template functions?