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

[Vadim Zeitlin]

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.

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

 I don't think it matter at all for a trivial inline function which should
be optimized away by the compiler at any optimization level.

GC>  - prefer "const&" to "&" because the parameter won't be modified

 This is true, of course, but OTOH using "T&" would forbid passing a
temporary to this function, which seems to make sense. Right now you could
write

        stifle_warning_for_unused_value(std::string("moo"));

even though it's completely useless.

GC> If we were proposing this for standardization, hence subjecting it
GC> to criticism from every angle, what would the best experts tell us
GC> that the parameter type ideally ought to be?

 I appreciate the usefulness of this thought experiment, but I'd still like
to mention that I don't think such function would have any chance of being
standardized. If the standard C++ ever addresses this problem, it will
almost certainly be done via an attribute on the variable itself.

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.

 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.

 As an example of why this could be useful, consider that making this
particular function constexpr wouldn't, of course, fix the clang warning --
why should it, when the problem is still there -- but it would also trigger
-Wmaybe-uninitialized from gcc now, just because it performs analysis for
it in the constexpr context, but not otherwise.

 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.

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, please see (or don't, as I'm
now trying to provide the branch names directly, so that you don't have to)
https://github.com/let-me-illustrate/lmi/pull/183 (the CI builds haven't
finished yet when I'm writing this, but I'm relatively confident that they
should pass).

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.

 Thanks,
VZ

pgpI5n0WtsHr6.pgp
Description: PGP signature