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

[Greg Chicares]

On 4/28/21 11:57 AM, Vadim Zeitlin wrote:
> On Wed, 28 Apr 2021 10:16:59 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> I feel uneasy about this change, for two reasons. First, before
> GC> this patch, this file contains two closely-related functions:
> GC>   template<typename T> inline void stifle_warning_for_unused_variable(T 
> const&)
> GC>   template<typename T> inline void stifle_warning_for_unused_value(T 
> const& t)
> GC>                                                                ^^^ 
> difference
> GC> and it's unsettling to remove 'const' from one but not the other.
> 
>  Yes, indeed, especially as it could be argued that "value" is more
> constant than "variable", not less. But then it's not really clear to me
> why do we need 2 functions in the first place, and the name of the latter
> function doesn't seem very accurate to me because it's used to suppress 
gcc
> -Wunused-but-set-variable warning, not "-Wunused-but-set-value" (which
> wouldn't make sense, of course). As I didn't want to open this can of
> worms, I've made what seemed to be the minimum acceptable change. But it
> turns out there is an even more minimal, and hence more acceptable, change.

I can't really say why there are two such functions, or why
they're named the way they are; or whether there should be
only one, and what its name should be. I don't remember,
and, alas, the commit history doesn't really make this clear.

There's a bit of a clue in this block comment:

/// Perhaps this function template's only legitimate use is within a
/// conditional-inclusion [16.1] block.

that documents stifle_warning_for_unused_variable(); but
in this apparently canonical snippet:

#else  // !defined LMI_X87
    stifle_warning_for_unused_variable(precision_mode);
    throw std::logic_error("Unable to set hardware precision.");
#endif // !defined LMI_X87

it appears that we can substitute stifle_warning_for_unused_value().

OTOH, stifle_warning_for_unused_value() has this documentation:

/// Taking the argument's address prevents this gcc warning:
///   "object of type [X] will not be accessed in void context"
/// for volatile types.

yet when I suppress it in one case, I get a different warning:
  error: variable ‘x’ set but not used [-Werror=unused-but-set-variable]
perhaps because gcc has changed somewhat since version 2.95 .

And I can't just read the implementations and readily say
where one would work and the other wouldn't, or even
whether such a distinguishing use case exists.

Neither occurs very often, so someone who has the time
could try replacing each one with the other, for example,
to see whether they're equivalent; or commenting out all
uses, to see what 21st-century warnings are elicited;
and then proposing a coherent replacement. It's worth
paying someone to do it, because this is forbiddingly
obscure, but I have to spend my own time elsewhere now.

> GC> so why should we not just add 'constexpr':
> GC> 
> GC>   template<typename T> inline void constexpr 
> stifle_warning_for_unused_value(T const& t)
> GC> 
> GC> but keep 'const'? We aren't going to modify the argument, so why
> GC> should we not guarantee that by writing 'const'?
> 
>  We could do this and, arguably, we should do this. But then we should do
> this and everywhere else where it could be done, it doesn't seem to single
> out this particular function for a particular treatment.

Agreed.

>  And this would definitely be a very useful thing to do because the
> compiler is _required_ to detect undefined behaviour in constexpr code. 
In
> fact, I have a long standing item in my TODO list (severity: "wishlist") to
> try running https://github.com/trailofbits/constexpr-everything on lmi. 
I
> didn't think at all to talk about this in the near future, but as you've
> brought this up yourself, please let me know if you think it would be worth
> bringing this item further to the top of my list.

'constexpr-everything' looks interesting. It should be on someone's
list, but I can't say off the top of my head where it lies in the
priority order.

>  And while I think that lmi should be pretty clean from undefined behaviour
> point of view (I ran it under UBSAN some time ago without finding many
> problems), I'm still willing to bet that constexpr-everything will find 
at
> least some occurrences of it and, of course, sprinkling the code with
> "constexpr" will prevent us from introducing it in the future.

Both clauses separated by "and, of course" seem compelling.

Does 'constexpr-everything' also have 'consteval-everything'
capabilities? Surely some lmi functions should be consteval;
I'm thinking of nonstd::power(T x, int n), at least for the
cases we really care about:
  double x = 2.0
  double x = 10.0
(where I was recently tempted to introduce lookup tables,
but that would be a terrible mistake if we can consteval
something like the existing code); and also this thing:  

    std::string look_up_scale_unit(int decimal_power)
        {
        return
             ( 0 == decimal_power) ? ""
            :( 3 == decimal_power) ? "thousand"
            :( 6 == decimal_power) ? "million"
            :( 9 == decimal_power) ? "billion"
            :(12 == decimal_power) ? "trillion"
            :(15 == decimal_power) ? "quadrillion"
            :(18 == decimal_power) ? "quintillion"
            : throw std::logic_error("Unnamed scaling unit.")
            ;
        }

> GC> Thus, the defect isn't in stifle_warning_for_unused_value()
> GC> at all; we'd get the same warning for any function foo():
> GC>     double volatile x;
> GC>     foo(x);
> GC> and I think the right thing to do here is:
> GC> 
> GC>     double volatile x;
> GC> -   stifle_warning_for_unused_value(x);
> GC>     // ...then assign some value to 'x'
> GC> +   stifle_warning_for_unused_value(x);
> 
>  Yes, you're right, it is a better fix, thanks. I've pushed it to the
> branch "stifle-unused-after-use" of vadz/lmi

Committed, tested, and pushed.

> GC> In any case, should we add 'constexpr' to both "stifle"
> GC> template functions?
> 
>  I think we should add it to all functions, template or not, that can be
> made constexpr. I'd rather not do it for just these functions nor do it
> right now, nor even next (as my next task is to ensure that lmi works as
> best as possible with wx 3.1.6 and 3.2.0, which shouldn't differ much from
> it), but we could prioritize this afterwards. But please let me know if 
you
> disagree and think it's higher priority than this, of course.

We agree completely.