`-Wcomma` flags Boost.Array code incorrectly

[mclow]

Bugzilla Link	47566
Version	unspecified
OS	All
CC	@DougGregor,@zygoloid,@Weverything

Extended Description

The following file:

#include <boost/array.hpp>

int main () {
	boost::array<int, 5> arr;
	return arr.size();
	}

when compiled with a recent clang using -Wcomma gives a warning.

/Users/marshall/Sources/Boost/main/boost/array.hpp:185:107: warning: possible misuse of comma operator here [-Wcomma]
            return i >= size() ? boost::throw_exception(std::out_of_range ("array<>: index out of range")), true : true;
                                                                                                          ^
/Users/marshall/Sources/Boost/main/boost/array.hpp:185:34: note: cast expression to void to silence warning
            return i >= size() ? boost::throw_exception(std::out_of_range ("array<>: index out of range")), true : true;
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                 static_cast<void>(                                                       )

The code that it is warning on is constexpr under C++11, and so must be a single expression. It looks like this:

return i >= size()
           ? boost::throw_exception(...), true
           : true;

The , true is necessary to make the two halves of the ?: expression have the same type.

I believe that the code is correct, and the warning is erroneous. Certainly the suggested "fix" is a complete non-starter.

[zygoloid]

Hm, what's wrong with the suggested cast to void:

return i >= size()
           ? static_cast<void>(boost::throw_exception(...)), true
           : true;

That wouldn't be allowed in a contant expression in some standard modes, but this branch of the ?: is presumably going to be non-constant anyway.

That said... inserting a cast to void of a void expression would seem a little silly. I wonder if we can reasonably disable this warning in all cases where the left-hand side is already of void type.

+rtrieu, what do you think? Are there cases we want to warn on where the left-hand side is of type void?

[mclow]

The goal here is to force a compile error in a constexpr context (boost_throw... is not constexpr), but in the case where that doesn't happen, both halves of the ternary operator need to have the same (or at least similar) types.

[Weverything]

The warning only ignores operands which are explicitly casted to void, because there's usually a better way to write things. For instance:

if (i >= size())
  boost::throw_exception(...);
return true;

This would be the better way to write the code if there was no requirement for a single statement. I don't see a better way to write this code without the comma operator and the explicit cast.

[mclow]

I don't see a better way to write this code without
the comma operator and the explicit cast.

I think that the way the code is written is fine.

[zygoloid]

The goal here is to force a compile error in a constexpr context
(boost_throw... is not constexpr), but in the case where that doesn't
happen, both halves of the ternary operator need to have the same (or at
least similar) types.

I think that goal would still be satisfied with the suggested change applied; in particular, clang is suggesting rewriting to

return i >= size()
           ? static_cast<void>(boost::throw_exception(...)), true
           : true;

(middle operand of type bool still), not to

return i >= size()
           ? static_cast<void>(boost::throw_exception(...), true)
           : true;

(middle operand of type void), so both operands to ?: would still be of the same type.

I don't see a better way to write this code without
the comma operator and the explicit cast.

I think that the way the code is written is fine.

Sure. But -Wcomma is, to some extent, a warning to enforce a particularly stringent style rule on the use of the comma operator, and as such it's not going to be applicable for users who don't want to enforce that style.

Perhaps we could do something to detect C++11-style constexpr functions (declared constexpr, body starts with return, ...) and disable -Wcomma in them, but I think even in that narrow case, use of commas is largely confined to cases where you want the evaluation to be non-constant anyway, and in those cases you can still insert a cast to void.

I think the better option, if we want to suppress this warning, would be to not require a cast to void if the left operand is already of type void. It seems unlikely that anything else was intended other than a comma expression if the left-hand side is already a void-typed expression. But we'll need to decide to what extent this is looking for programmer mistakes versus enforcing a coding style.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Marshall Clow (mclow)

| | | | --- | --- | | Bugzilla Link | [47566](https://llvm.org/bz47566) | | Version | unspecified | | OS | All | | CC | @DougGregor,@zygoloid,@Weverything |

Extended Description

The following file:

#include <boost/array.hpp>

int main () {
	boost::array<int, 5> arr;
	return arr.size();
	}

when compiled with a recent clang using -Wcomma gives a warning.

/Users/marshall/Sources/Boost/main/boost/array.hpp:185:107: warning: possible misuse of comma operator here [-Wcomma]
            return i >= size() ? boost::throw_exception(std::out_of_range ("array<>: index out of range")), true : true;
                                                                                                          ^
/Users/marshall/Sources/Boost/main/boost/array.hpp:185:34: note: cast expression to void to silence warning
            return i >= size() ? boost::throw_exception(std::out_of_range ("array<>: index out of range")), true : true;
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                 static_cast<void>(                                                       )

The code that it is warning on is constexpr under C++11, and so must be a single expression. It looks like this:

return i >= size()
           ? boost::throw_exception(...), true
           : true;

The , true is necessary to make the two halves of the ?: expression have the same type.

I believe that the code is correct, and the warning is erroneous. Certainly the suggested "fix" is a complete non-starter.