4

Is there a way to fix the following problem:

This code produces a C4702 warning 'unreachable code' (on VC++ 15.8 with /std:c++17)

template <typename T, typename VariantType>
inline bool MatchMonostate( VariantType& variant )
{
    SUPPRESS_C4100( variant );
    if constexpr ( std::is_same_v<T, std::monostate> )
    {
        variant = std::monostate();
        return true;
    }
    return false;  // !!! unreachable if the above is true !!! => C4702
}

to suppress the C4100 'unreferenced formal parameter' warning, I'm already using the trick

#define SUPPRESS_C4100(x) ((void)x)

The simple idea of adding

    else
    {
        return false;
    }

results in warning C4715 'not all control paths return a value' instead.

old123987
  • 121
  • 6
  • You do not need the `SUPRESS_C4100` "trick". See here: https://wandbox.org/permlink/zewgEt073tRV3dJV .. same is here on my machine with MSVC 19.15.26726 –  Sep 09 '18 at 23:23
  • Thx for pointing this out. Just using `bool MatchMonostate( [[maybe_unused]] VariantType& variant )` gets rid of C4100. – old123987 Sep 10 '18 at 12:02
  • yes that works too. Although latest releases of the "big three" do no need even that ... –  Sep 10 '18 at 12:22
  • Not sure which is your 3rd. My VC++ 15.8.3 with /std:c++latest will issue C4100 without `[[maybe_unused]]` – old123987 Sep 10 '18 at 15:34
  • ... I might be missing something .. please go to your devl command prompt and do "`cl /Bv`" .That gives the version nums. mine is 19.15.26726.0 ... I shall re-try in an empty project ... –  Sep 11 '18 at 18:24
  • just made an empty project with `/std:c++17` , and indeed no warnings with `/W3`. `/W4` and `/Wall` do emit C4100. Devl command line and `cl /Bv` produce `Version: 19.15.26726.0`, on my machine. –  Sep 12 '18 at 07:47
  • Without else I get the C4702; with 'else' solution it's gone. The unreferenced argument parameter must be suppressed. – gast128 Dec 21 '19 at 15:31

2 Answers2

6

It's unreachable because for a given expansion of the template based on the template arguments the function will only ever pass the condition and return true or fail and return false. There is no case where it could go either way for the same type. It's essentially expanding to

if (true) {
  return true;
}
return false; // Obviously will never happen

I'd rewrite it to only have a single return statement.

template <typename T, typename VariantType>
inline bool MatchMonostate( VariantType& variant )
{
    SUPPRESS_C4100( variant );
    bool retval = false;
    if constexpr ( std::is_same_v<T, std::monostate> )
    {
        variant = std::monostate();
        retval = true;
    }
    return retval;
}

Also, in the case where the condition is true variant is not unused. You may want to move that line that suppresses the warning (which basically turns into (void)variant) to an else statement.

Moohasha
  • 245
  • 1
  • 13
  • 1
    Still, I would have thought, a modern compiler could figure this out by itself – old123987 Sep 09 '18 at 12:58
  • @old123987 the compiler did figure it out and warned you that the line was unreachable. =P – Moohasha Sep 09 '18 at 12:59
  • You mean aside from answering the question? The question was about a line being unreachable. This answer explained why and corrected it. – Moohasha Sep 09 '18 at 21:36
  • @ChefGladiator Casting a thing to void to suppress 'unused variable' warning is widely used pre-C++17 (and in C). It's not standard, but I wouldn't say it's *"as far as one can get from standard portable C++"*. As far as I know, it's fairly portable and should work on most decent compilers. – HolyBlackCat Sep 09 '18 at 21:59
  • Where did OP say he was trying to write portable code? – Moohasha Sep 09 '18 at 22:15
  • Also the part `if (true) { return true; } return false; // Obviously will never happen` is wrong. 'return false' will obviously happen whenever T is not a monostate type –  Sep 10 '18 at 06:53
  • I'd use `else` and can't figure out why the compiler still warns when you do – Lightness Races in Orbit Sep 10 '18 at 12:27
  • @LightnessRacesinOrbit yup, I did that in my answers. Also, it seems the latest releases of clang/gcc/msvc have sorted this out and no warnings are being produced. https://wandbox.org/permlink/zewgEt073tRV3dJV –  Sep 10 '18 at 12:43
  • The part `if (true) {return true;} return false; // Obviously will never happen` is _not_ wrong, that's the entire problem that's causing the warning about unreachable code. Remember this is a template function, and as such will get expanded differently for every type. The condition is based entirely on the type specified in the template, and thus the condition will _always_ resolve true or false depending on the type template type. I only illustrated the first case because that results in the `return false;` line being unreachable, hence the question asked. – Moohasha Sep 10 '18 at 13:10
  • I work in a group whose coding standards _strongly_ encourage a single exit point to every function, and over time it's become a bit of a personal preference for me as I always know where the function will return. In this case, because the result of the if condition is known at compile time, adding an else will still result in unreachable code (though the compiler _should_ optimize it out in that case). That's why I went with a return value and a single return statement. – Moohasha Sep 10 '18 at 13:16
  • @Moohasha Clearly Chef was referring to the comment's more global veracity, which is indeed in question. Remember, that comment exists in every specialisation ;) – Lightness Races in Orbit Sep 10 '18 at 13:16
  • Well that's fine but I do not work in such a group ;) – Lightness Races in Orbit Sep 10 '18 at 13:17
  • Which I why I said it's become a personal preference; I was explaining why _I_ did it that way to someone who said they would have done it differently. – Moohasha Sep 10 '18 at 13:18
  • @Moohasha please see my answer on this exact issue. This is actually two functions. Each with a single exit point. –  Sep 11 '18 at 12:27
  • @HolyBlackCat .. Sory for a late reply. "casting a 'thing' to void" aka `((void)x);` will execute the "thing" aka `x` if it happens to be an expression. –  Sep 11 '18 at 12:30
1

As the direct answer to the direct question. On the subject of if constexpr. Consider this:

template <typename T, typename ... params >
 inline bool match_monostate
  (std::variant<params ...> & variant) noexcept    
{
 if constexpr (std::is_same_v<T, std::monostate>)
 {
     variant = std::monostate{} ;
 //  compiles only if called with variant
 //  whose one alternative is std::monostate
     return true;
 }
 else {
    return false;
 }
}

Depending on the bool result of the if constexpr expression, compiler actually produces two functions. This version is produced when if constexpr() yields true:

  template <typename T, typename ... params >
 inline bool 
 match_monostate  (std::variant<params ...> & variant)  noexcept
{
    variant = std::monostate{} ;
//  compiles only if called with variant
//  whose one alternative is std::monostate
    return true;
}

This version is produced when if constexpr() yields false:

template <typename T, typename ... params >
 inline bool 
 match_monostate  (std::variant<params ...> & variant)  noexcept
{
    return false;
}

The second version might emit warnings about unused argument. But (it seems) not if using the latest versions of clang/gcc/msvc. For older compilers as "old123987" also pointed out one can add the standard attribute to the signature. Like so:

 template <typename T, typename ... params >
 inline bool 
 match_monostate  ([[maybe_unused]] std::variant<params ...> & variant) ;

That will stop the warning emitting.

  • Do you see any upside/downside in using std::variant& vs just VariantType& ? (besides type safety) – old123987 Sep 10 '18 at 15:42
  • I can not see any obvious downside to the `match_monostate` signature, as presented. It will also stop you doing for example this: `match_monostate( variant{});` or calling it with const instances ... –  Sep 11 '18 at 07:17
  • As an anti jokers measure you can delete this declaration: ` template inline bool match_monostate ( std::variant && variant) ; = delete`, to stop them calling it with moved instances `match_monostate( move(var_))` –  Sep 11 '18 at 07:36