assert: Support types without operator== (int) [BZ #21972]

Message ID c5404f0c-3b3d-7ac1-556d-8d8888785d6c@redhat.com
State Committed
Headers

Commit Message

Florian Weimer Aug. 18, 2017, 6:27 p.m. UTC
  On 08/18/2017 07:40 PM, Florian Weimer wrote:
> On 08/18/2017 07:30 PM, Paul Eggert wrote:
>> Florian Weimer wrote:
>>>> Florian Weimer wrote:
>>>>> + ((void) sizeof (static_cast<bool> (expr)), __extension__ ({ \
>>>> Can we instead replace 'sizeof ((expr) == 0)' with 'sizeof !(expr)'?
>>>> That works in C, and if it also works in C++ it would avoid the need
>>>> for the #ifdef __cplusplus.
>>> It does not work in C++ because operator! could have been overloaded
>>> or deleted.
>>
>> OK, then how about 'sizeof ((expr) ? 1 : 0)'? This should work because
>> ?: cannot be overloaded.
> 
> Oh, this works.  Updated patch attached.

I think it's still valuable to use static_cast<bool> (expr) for C++
because we don't need any extensions (except __FUNCTION__ for early C++)
because the parentheses do not count in this context; assert (a = 1)
still results in a warning.

Another patch attached.

Thanks,
Florian
  

Comments

Paul Eggert Aug. 18, 2017, 7:17 p.m. UTC | #1
Florian Weimer wrote:
> I think it's still valuable to use static_cast<bool> (expr) for C++
> because we don't need any extensions (except __FUNCTION__ for early C++)
> because the parentheses do not count in this context; assert (a = 1)
> still results in a warning.

OK, but in that case the C version shouldn't use 'sizeof ((expr) ? 1 : 0)' 
either, right? As that would suppress the warning in the C case. So it sounds 
like your original patch was right after all (and sorry about the noise...).
  
Florian Weimer Aug. 18, 2017, 7:57 p.m. UTC | #2
On 08/18/2017 09:17 PM, Paul Eggert wrote:
> Florian Weimer wrote:
>> I think it's still valuable to use static_cast<bool> (expr) for C++
>> because we don't need any extensions (except __FUNCTION__ for early C++)
>> because the parentheses do not count in this context; assert (a = 1)
>> still results in a warning.
> 
> OK, but in that case the C version shouldn't use 'sizeof ((expr) ? 1 :
> 0)' either, right? As that would suppress the warning in the C case.

We are interested in two warnings:

  assert (a = 1);
  assert (({ 1; }));

The first one is about the assignment (and should happen in as many
cases as possible), and the second one is about pedantic warnings in
strict standard mode.

For GNU C, we need to use __extension__ ({ if (E) …  }) to evaluate the
expression without extra parentheses in a Boolean context to get the
first warning, but the __extension__ inhibits the warning for the second
construct, so we expand it twice.  The expansion in the sizeof will not
warn about the assignment, but that's fine because we catch that with
the other expansion.

> So
> it sounds like your original patch was right after all (and sorry about
> the noise...).

I think the last patch is superior because we get the first warning even
in strict C++ mode.

I'll delete a couple more operators before committing it.

Thanks,
Florian
  

Patch

assert: Support types without operator== (int) [BZ #21972]

2017-08-18  Florian Weimer  <fweimer@redhat.com>

	[BZ #21972]
	* assert/assert.h (assert): Use static_cast (bool) for C++.
	Use the ternary operator in the warning branch for GNU C.
	* assert/Makefile (tests): Add tst-assert-c++, tst-assert-g++.
	(CFLAGS-tst-assert-c++.o): Compile in C++11 mode.
	(CFLAGS-tst-assert-g++.o): Compile in GnU C++11 mode.
	(LDLIBS-tst-assert-c++, LDLIBS-tst-assert-g++): Link with libstdc++.
	* assert/tst-assert-c++.cc, assert/tst-assert-g++.cc: New files.

diff --git a/assert/Makefile b/assert/Makefile
index 1c3be9b01f..9ec1be81a9 100644
--- a/assert/Makefile
+++ b/assert/Makefile
@@ -25,6 +25,15 @@  include ../Makeconfig
 headers	:= assert.h
 
 routines := assert assert-perr __assert
-tests := test-assert test-assert-perr
+tests := test-assert test-assert-perr tst-assert-c++ tst-assert-g++
 
 include ../Rules
+
+ifeq ($(have-cxx-thread_local),yes)
+CFLAGS-tst-assert-c++.o = -std=c++11
+LDLIBS-tst-assert-c++ = -lstdc++
+CFLAGS-tst-assert-g++.o = -std=gnu++11
+LDLIBS-tst-assert-g++ = -lstdc++
+else
+tests-unsupported += tst-assert-c++ tst-assert-g++
+endif
diff --git a/assert/assert.h b/assert/assert.h
index 6801cfeb10..640c95c063 100644
--- a/assert/assert.h
+++ b/assert/assert.h
@@ -85,7 +85,12 @@  __END_DECLS
 /* When possible, define assert so that it does not add extra
    parentheses around EXPR.  Otherwise, those added parentheses would
    suppress warnings we'd expect to be detected by gcc's -Wparentheses.  */
-# if !defined __GNUC__ || defined __STRICT_ANSI__
+# if defined __cplusplus
+#  define assert(expr)							\
+     (static_cast <bool> (expr)						\
+      ? void (0)							\
+      : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
+# elif !defined __GNUC__ || defined __STRICT_ANSI__
 #  define assert(expr)							\
     ((expr)								\
      ? __ASSERT_VOID_CAST (0)						\
@@ -93,12 +98,11 @@  __END_DECLS
 # else
 /* The first occurrence of EXPR is not evaluated due to the sizeof,
    but will trigger any pedantic warnings masked by the __extension__
-   for the second occurrence.  The explicit comparison against zero is
-   required to support function pointers and bit fields in this
-   context, and to suppress the evaluation of variable length
-   arrays.  */
+   for the second occurrence.  The ternary operator is required to
+   support function pointers and bit fields in this context, and to
+   suppress the evaluation of variable length arrays.  */
 #  define assert(expr)							\
-  ((void) sizeof ((expr) == 0), __extension__ ({			\
+  ((void) sizeof ((expr) ? 1 : 0), __extension__ ({			\
       if (expr)								\
         ; /* empty */							\
       else								\
diff --git a/assert/tst-assert-c++.cc b/assert/tst-assert-c++.cc
new file mode 100644
index 0000000000..c001bfff49
--- /dev/null
+++ b/assert/tst-assert-c++.cc
@@ -0,0 +1,72 @@ 
+/* Tests for interactions between C++ and assert.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <assert.h>
+
+/* The C++ standard requires that if the assert argument is a constant
+   subexpression, then the assert itself is one, too.  */
+constexpr int
+check_constexpr ()
+{
+  return (assert (true), 1);
+}
+
+/* Objects of this class can be contextually converted to bool, but
+   cannot be compared to int.  */
+struct no_int
+{
+  no_int () = default;
+  no_int (const no_int &) = delete;
+
+  explicit operator bool () const
+  {
+    return true;
+  }
+};
+
+/* This class tests that operator== is not used by assert.  */
+struct bool_and_int
+{
+  bool_and_int () = default;
+  bool_and_int (const no_int &) = delete;
+
+  explicit operator bool () const
+  {
+    return true;
+  }
+
+  template <class T> bool operator== (T) const; /* No definition.  */
+};
+
+static int
+do_test ()
+{
+  {
+    no_int value;
+    assert (value);
+  }
+
+  {
+    bool_and_int value;
+    assert (value);
+  }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/assert/tst-assert-g++.cc b/assert/tst-assert-g++.cc
new file mode 100644
index 0000000000..8c06402825
--- /dev/null
+++ b/assert/tst-assert-g++.cc
@@ -0,0 +1,19 @@ 
+/* Tests for interactions between C++ and assert.  GNU C++11 version.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <tst-assert-c++.cc>