[PATCH] cpp: allow implicit conversion from boolean to bool

[Tobias Grosser]

This allows the use of isl::boolean values as first operand of
__builtin_assume(long, long) without the need for explicit casts. This is
useful as OSX uses __builtin_assume in its implementation of assert() to
indicate to the compiler that the condition that is verified is true with high
probability. Without allowing implicit conversions to bool, the use
of isl::boolean without an explicit cast to bool results in
a compilation failure on OSX. As explicit casts seem too verbose to be used in
each assert call, this change allows implicit conversions from boolean
to bool.

Originally, implicit conversions from boolean to bool were disallowed to start
off with a minimal C++ interface that is as explicit as possible. This
minimal interface was understood to be widened in case practical use
cases motivate an extended interface. Extending the interface to avoid
many explicit casts in common situtations is exactly such a practical
use case.

Also update the tests. Incorrectly, the tests already assumed that
'assert(b_true)' would test implicit conversions. This was not the case
and new test cases have been added that test implicit conversion to both
bool and long.

Suggested-by: Siddharth Bath <siddhar...@research.iiit.ac.in>
Signed-off-by: Tobias Grosser <tob...@grosser.es>
---
interface/isl.h.top | 2 +-
interface/isl_test_cpp.cpp | 17 +++++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/interface/isl.h.top b/interface/isl.h.top
index 37749c2..56312d2 100644
--- a/interface/isl.h.top
+++ b/interface/isl.h.top
@@ -55,7 +55,7 @@ public:
bool is_false() const { return val == isl_bool_false; }
bool is_true() const { return val == isl_bool_true; }

- explicit operator bool() const {
+ operator bool() const {
ISLPP_ASSERT(!is_error(), "IMPLEMENTATION ERROR: Unhandled error state");
return is_true();
}
diff --git a/interface/isl_test_cpp.cpp b/interface/isl_test_cpp.cpp
index 6059208..e85d770 100644
--- a/interface/isl_test_cpp.cpp
+++ b/interface/isl_test_cpp.cpp
@@ -188,6 +188,7 @@ void test_return_int(isl::ctx ctx)
* respectively
* - Explicit conversion to 'bool'
* - Implicit conversion to 'bool'
+ * - Implicit conversion to 'long'
* - The complement operator
* - Explicit construction from 'true' and 'false'
* - Explicit construction form isl_bool
@@ -219,6 +220,22 @@ void test_return_bool(isl::ctx ctx)

assert(b_true);

+ bool use_in_bool_expr = b_true && true;
+ bool use_in_bool_expr_2 = true && b_true;
+
+ assert(use_in_bool_expr == use_in_bool_expr_2 &&
+ use_in_bool_expr == true);
+
+ bool bool_true = b_true;
+ bool bool_false = b_false;
+
+ assert(bool_true && !bool_false);
+
+ long long_true = b_true;
+ long long_false = b_false;
+
+ assert(long_true && !long_false);
+
assert((!b_false).is_true());
assert((!b_true).is_false());
assert((!b_error).is_error());
--
2.9.3

[Sven Verdoolaege]

On Wed, May 31, 2017 at 10:48:28AM +0200, Tobias Grosser wrote:
> This allows the use of isl::boolean values as first operand of
> __builtin_assume(long, long) without the need for explicit casts. This is
> useful as OSX uses __builtin_assume in its implementation of assert() to
> indicate to the compiler that the condition that is verified is true with high
> probability. Without allowing implicit conversions to bool, the use
> of isl::boolean without an explicit cast to bool results in
> a compilation failure on OSX. As explicit casts seem too verbose to be used in
> each assert call, this change allows implicit conversions from boolean
> to bool.

I thought we wanted the conversion to be explicit such that
users would be aware that this conversion may lose information.
I suppose you should just do an explicit cast in your assert.
If that's too verbose, then you can add a macro to hide the cast,
with the name of the macro indicating that you are aware of
the issue.

I'm also interested in Michael's thoughts.

> Also update the tests. Incorrectly, the tests already assumed that
> 'assert(b_true)' would test implicit conversions. This was not the case
> and new test cases have been added that test implicit conversion to both
> bool and long.

I missed that. This should be fixed regardless.

skimo

[Tobias Grosser]

On Wed, May 31, 2017, at 06:00 PM, Sven Verdoolaege wrote:
> On Wed, May 31, 2017 at 10:48:28AM +0200, Tobias Grosser wrote:
> > This allows the use of isl::boolean values as first operand of
> > __builtin_assume(long, long) without the need for explicit casts. This is
> > useful as OSX uses __builtin_assume in its implementation of assert() to
> > indicate to the compiler that the condition that is verified is true with high
> > probability. Without allowing implicit conversions to bool, the use
> > of isl::boolean without an explicit cast to bool results in
> > a compilation failure on OSX. As explicit casts seem too verbose to be used in
> > each assert call, this change allows implicit conversions from boolean
> > to bool.
>
> I thought we wanted the conversion to be explicit such that
> users would be aware that this conversion may lose information.

Right. Minimizing surprises was one of the goals and we tried to make
all conversations as explicit as possible.

> I suppose you should just do an explicit cast in your assert.
> If that's too verbose, then you can add a macro to hide the cast,
> with the name of the macro indicating that you are aware of
> the issue.

I think me adding a cast does not solve the underlying problem that the
C++ bindings act very surprising in a common situation.
Being able to use a common construct such as an assert without problems
on all major operating systems not even seeing compiler warnings, but
failing compilations on OSX is clearly surprising and will cause new
users to write broken code without even knowing.

I am aware that we trade a slightly reduced explicitness here for less
surprising behavior of the isl C++ bindings. However, I think this is a
good trade-off. Users of isl C already have the risk of implicit
conversions and are aware that they might happen. In case they are used
in C++ and they indeed happen at run-time an error will be printed. This
is a clear improvement over the C interface without requiring

> I'm also interested in Michael's thoughts.

Me as well.

> > Also update the tests. Incorrectly, the tests already assumed that
> > 'assert(b_true)' would test implicit conversions. This was not the case
> > and new test cases have been added that test implicit conversion to both
> > bool and long.
>
> I missed that. This should be fixed regardless.

OK. I will pull this into a separate commit, but wait until we conclude
on the other part of the discussion.

Best,
Tobias

[Michael Kruse]

2017-05-31 18:00 GMT+02:00 Sven Verdoolaege <skim...@kotnet.org>:
> On Wed, May 31, 2017 at 10:48:28AM +0200, Tobias Grosser wrote:
>> This allows the use of isl::boolean values as first operand of
>> __builtin_assume(long, long) without the need for explicit casts. This is
>> useful as OSX uses __builtin_assume in its implementation of assert() to
>> indicate to the compiler that the condition that is verified is true with high
>> probability. Without allowing implicit conversions to bool, the use
>> of isl::boolean without an explicit cast to bool results in
>> a compilation failure on OSX. As explicit casts seem too verbose to be used in
>> each assert call, this change allows implicit conversions from boolean
>> to bool.
>
> I thought we wanted the conversion to be explicit such that
> users would be aware that this conversion may lose information.
> I suppose you should just do an explicit cast in your assert.
> If that's too verbose, then you can add a macro to hide the cast,
> with the name of the macro indicating that you are aware of
> the issue.
>
> I'm also interested in Michael's thoughts.

bool is implicitly convertible to any integer value, which can give
very surprising results. An idiom [1] is been invented to avoid it. It
was also the reason the C++ committee introduces explicit conversions
[2]. It was considered enough of a problem to justify a language
change.

I searched the net for others having this problem on OSX, but couldn't
find anything. Others must have encountered the problem as well, no?
LLVM itself uses it (e.g. llvm::Error, llvm::Optional) as well as
assertions. Is there no place in all of LLVM's source that uses such
an explicitly-convertible object in an assert? I find that hard to
believe.

There are also easy workarounds: assert(!!b_true), assert(b_true &&
"What's going on"), assert(b_true == true) (I think we need to add
operator overloads to support the latter);

According to [3] ([assertions.assert] 1.2), assert must accept an
argument that is "contextually converted to bool" (that is, must
accept explicit-only conversions to bool). I conclude that Siddarth's
version of OSX (do you mean Apple clang? OSX is an operating system)
is not standard-conforming, at least not to C++17.

To summarize, I am strongly against making the bool conversion implicit.

Michael

[1] https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool
[2] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2333.html
[3] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf

[Sven Verdoolaege]

On Thu, Jun 01, 2017 at 02:39:04PM +0200, Michael Kruse wrote:
> There are also easy workarounds: assert(!!b_true), assert(b_true &&
> "What's going on"), assert(b_true == true) (I think we need to add
> operator overloads to support the latter);

I'd write that as assert(b_true.is_true()).

> To summarize, I am strongly against making the bool conversion implicit.

Thanks. I agree with the conclusion.

skimo

[Tobias Grosser]

On Thu, Jun 1, 2017, at 02:39 PM, Michael Kruse wrote:
> 2017-05-31 18:00 GMT+02:00 Sven Verdoolaege <skim...@kotnet.org>:
> > On Wed, May 31, 2017 at 10:48:28AM +0200, Tobias Grosser wrote:
> >> This allows the use of isl::boolean values as first operand of
> >> __builtin_assume(long, long) without the need for explicit casts. This is
> >> useful as OSX uses __builtin_assume in its implementation of assert() to
> >> indicate to the compiler that the condition that is verified is true with high
> >> probability. Without allowing implicit conversions to bool, the use
> >> of isl::boolean without an explicit cast to bool results in
> >> a compilation failure on OSX. As explicit casts seem too verbose to be used in
> >> each assert call, this change allows implicit conversions from boolean
> >> to bool.
> >
> > I thought we wanted the conversion to be explicit such that
> > users would be aware that this conversion may lose information.
> > I suppose you should just do an explicit cast in your assert.
> > If that's too verbose, then you can add a macro to hide the cast,
> > with the name of the macro indicating that you are aware of
> > the issue.
> >
> > I'm also interested in Michael's thoughts.
>
> bool is implicitly convertible to any integer value, which can give
> very surprising results. An idiom [1] is been invented to avoid it. It
> was also the reason the C++ committee introduces explicit conversions
> [2]. It was considered enough of a problem to justify a language
> change.

I think our use case is different. The original risk one wanted to avoid
was that two smart pointers pointing to different kind of classes can be
compared, which potentially compares entirely different objects. The
object we convert to bool is a boolean value. The only information that
can get lost is the fact that there was an error value and this will not
be silently lost, but an explicit error will be raised.

Do you have some specific piece of code in mind that would break
unexpectedly.

> I searched the net for others having this problem on OSX, but couldn't
> find anything. Others must have encountered the problem as well, no?
> LLVM itself uses it (e.g. llvm::Error, llvm::Optional) as well as
> assertions. Is there no place in all of LLVM's source that uses such
> an explicitly-convertible object in an assert? I find that hard to
> believe.

I tried to use llvm::Error as an argument to __builtin_expect() and it
breaks the
same way.

> There are also easy workarounds: assert(!!b_true)

This is still of type isl::boolean and won't work.

> assert(b_true && "What's going on"),

This works. It is probably best practice to add a comment anyhow. Still,
this does not solve the surprising effect other people will have with
the bindings.

> assert(b_true == true) (I think we need to add
> operator overloads to support the latter);

I personally think this is not very beautiful. I like Sven's suggestion
or the one beforehand better.

> According to [3] ([assertions.assert] 1.2), assert must accept an
> argument that is "contextually converted to bool" (that is, must
> accept explicit-only conversions to bool). I conclude that Siddarth's
> version of OSX (do you mean Apple clang? OSX is an operating system)
> is not standard-conforming, at least not to C++17.

Which page is this? I can only find "4 In a static_assert-declaration
the constant-expression shall be a constant expression (5.19) that can
be contextually converted to bool", which does not apply here.

"man assert"

POSIX.1-2001, POSIX.1-2008, C89, C99. In C89, expression is required to
be of type
int and undefined behavior results if it is not, but in C99 it may
have any scalar
type.

> To summarize, I am strongly against making the bool conversion implicit.

It seems you both are very convinced. I wrote the email mostly for
completeness. Let me know if anybody changed their mind, otherwise I
assume this discussion is closed.
(I still dislike that it will be easy for people to write code that runs
on linux and breaks on osX)

Best,
Tobias