Cleanup of DOCTEST_DO_BINARY_EXPRESSION_COMPARISON. Fixes #651 (#652)

* Cleanup of DOCTEST_DO_BINARY_EXPRESSION_COMPARISON. Fixes #651

* Add test

* Remove redundant assertion and default member initializer

* Add extra test

* Suppression

Co-authored-by: Salvage <29021710+Saalvage@users.noreply.github.com>

11 files changed, 91 insertions, 42 deletions

diff --git a/doctest/doctest.h b/doctest/doctest.h
index 08b4e2f2..0d7b8bb4 100644
--- a/doctest/doctest.h
+++ b/doctest/doctest.h
@@ -735,7 +735,7 @@ namespace assertType {
         DT_WARN_THROWS_WITH    = is_throws_with | is_warn,
         DT_CHECK_THROWS_WITH   = is_throws_with | is_check,
         DT_REQUIRE_THROWS_WITH = is_throws_with | is_require,
-        
+
         DT_WARN_THROWS_WITH_AS    = is_throws_with | is_throws_as | is_warn,
         DT_CHECK_THROWS_WITH_AS   = is_throws_with | is_throws_as | is_check,
         DT_REQUIRE_THROWS_WITH_AS = is_throws_with | is_throws_as | is_require,
@@ -1330,17 +1330,8 @@ DOCTEST_CLANG_SUPPRESS_WARNING_WITH_PUSH("-Wunused-comparison")
 
 #define DOCTEST_DO_BINARY_EXPRESSION_COMPARISON(op, op_str, op_macro)                              \
     template <typename R>                                                                          \
-    DOCTEST_NOINLINE SFINAE_OP(Result,op) operator op(const R&& rhs) {                             \
-    bool res = op_macro(doctest::detail::forward<const L>(lhs), doctest::detail::forward<const R>(rhs)); \
-        if(m_at & assertType::is_false)                                                            \
-            res = !res;                                                                            \
-        if(!res || doctest::getContextOptions()->success)                                          \
-            return Result(res, stringifyBinaryExpr(lhs, op_str, rhs));                             \
-        return Result(res);                                                                        \
-    }                                                                                              \
-    template <typename R ,typename types::enable_if<!doctest::detail::types::is_rvalue_reference<R>::value, void >::type* = nullptr> \
-    DOCTEST_NOINLINE SFINAE_OP(Result,op) operator op(const R& rhs) {                              \
-    bool res = op_macro(doctest::detail::forward<const L>(lhs), rhs);                              \
+    DOCTEST_NOINLINE SFINAE_OP(Result,op) operator op(R&& rhs) {                                   \
+    bool res = op_macro(doctest::detail::forward<const L>(lhs), doctest::detail::forward<R>(rhs)); \
         if(m_at & assertType::is_false)                                                            \
             res = !res;                                                                            \
         if(!res || doctest::getContextOptions()->success)                                          \
@@ -1792,7 +1783,7 @@ DOCTEST_CLANG_SUPPRESS_WARNING_POP
 
     DOCTEST_INTERFACE void registerExceptionTranslatorImpl(const IExceptionTranslator* et);
 
-    // ContextScope base class used to allow implementing methods of ContextScope 
+    // ContextScope base class used to allow implementing methods of ContextScope
     // that don't depend on the template parameter in doctest.cpp.
     struct DOCTEST_INTERFACE ContextScopeBase : public IContextScope {
         ContextScopeBase(const ContextScopeBase&) = delete;
@@ -1871,7 +1862,7 @@ DOCTEST_MSVC_SUPPRESS_WARNING_POP
         bool log();
         void react();
     };
-    
+
     template <typename L>
     ContextScope<L> MakeContextScope(const L &lambda) {
         return ContextScope<L>(lambda);
diff --git a/doctest/parts/doctest_fwd.h b/doctest/parts/doctest_fwd.h
index 1a51dee1..5cbaeab2 100644
--- a/doctest/parts/doctest_fwd.h
+++ b/doctest/parts/doctest_fwd.h
@@ -732,7 +732,7 @@ namespace assertType {
         DT_WARN_THROWS_WITH    = is_throws_with | is_warn,
         DT_CHECK_THROWS_WITH   = is_throws_with | is_check,
         DT_REQUIRE_THROWS_WITH = is_throws_with | is_require,
-        
+
         DT_WARN_THROWS_WITH_AS    = is_throws_with | is_throws_as | is_warn,
         DT_CHECK_THROWS_WITH_AS   = is_throws_with | is_throws_as | is_check,
         DT_REQUIRE_THROWS_WITH_AS = is_throws_with | is_throws_as | is_require,
@@ -1327,17 +1327,8 @@ DOCTEST_CLANG_SUPPRESS_WARNING_WITH_PUSH("-Wunused-comparison")
 
 #define DOCTEST_DO_BINARY_EXPRESSION_COMPARISON(op, op_str, op_macro)                              \
     template <typename R>                                                                          \
-    DOCTEST_NOINLINE SFINAE_OP(Result,op) operator op(const R&& rhs) {                             \
-    bool res = op_macro(doctest::detail::forward<const L>(lhs), doctest::detail::forward<const R>(rhs)); \
-        if(m_at & assertType::is_false)                                                            \
-            res = !res;                                                                            \
-        if(!res || doctest::getContextOptions()->success)                                          \
-            return Result(res, stringifyBinaryExpr(lhs, op_str, rhs));                             \
-        return Result(res);                                                                        \
-    }                                                                                              \
-    template <typename R ,typename types::enable_if<!doctest::detail::types::is_rvalue_reference<R>::value, void >::type* = nullptr> \
-    DOCTEST_NOINLINE SFINAE_OP(Result,op) operator op(const R& rhs) {                              \
-    bool res = op_macro(doctest::detail::forward<const L>(lhs), rhs);                              \
+    DOCTEST_NOINLINE SFINAE_OP(Result,op) operator op(R&& rhs) {                                   \
+    bool res = op_macro(doctest::detail::forward<const L>(lhs), doctest::detail::forward<R>(rhs)); \
         if(m_at & assertType::is_false)                                                            \
             res = !res;                                                                            \
         if(!res || doctest::getContextOptions()->success)                                          \
@@ -1789,7 +1780,7 @@ DOCTEST_CLANG_SUPPRESS_WARNING_POP
 
     DOCTEST_INTERFACE void registerExceptionTranslatorImpl(const IExceptionTranslator* et);
 
-    // ContextScope base class used to allow implementing methods of ContextScope 
+    // ContextScope base class used to allow implementing methods of ContextScope
     // that don't depend on the template parameter in doctest.cpp.
     struct DOCTEST_INTERFACE ContextScopeBase : public IContextScope {
         ContextScopeBase(const ContextScopeBase&) = delete;
@@ -1868,7 +1859,7 @@ DOCTEST_MSVC_SUPPRESS_WARNING_POP
         bool log();
         void react();
     };
-    
+
     template <typename L>
     ContextScope<L> MakeContextScope(const L &lambda) {
         return ContextScope<L>(lambda);
diff --git a/examples/all_features/decomposition.cpp b/examples/all_features/decomposition.cpp
index c48f8a4a..2713971b 100644
--- a/examples/all_features/decomposition.cpp
+++ b/examples/all_features/decomposition.cpp
@@ -24,4 +24,40 @@ static MoveOnly genType(bool b) {
 TEST_CASE("Move Only Type") {
     CHECK(genType(true));
     CHECK(genType(false));
+
+    MoveOnly a{ 0 };
+    CHECK(a);
+}
+
+
+struct int_pointer {
+    int* p = nullptr;
+    int_pointer() = default;
+
+    // non-const ref used in constructor
+    // we don't want to accidentally construct a dangling pointer from a temporary
+    int_pointer(int& i) : p(&i) { }
+
+    // NOLINTNEXTLINE(readability-make-member-function-const)
+    explicit operator bool() { return !!p; }
+    int val() const { return *p; }
+};
+
+struct int_holder {
+    int i;
+    bool operator==(int_pointer p) const {
+        if (!p) return false;
+        return i == p.val();
+    }
+};
+
+DOCTEST_MSVC_SUPPRESS_WARNING_WITH_PUSH(4866) // eval order
+TEST_CASE("Impl cast from non-const value") {
+    int_holder h{ 8 };
+    int i = 8;
+
+    // this comparison is safe
+    // int_pointer can be implicitly constructed from this int here
+    CHECK(h == i);
 }
+DOCTEST_MSVC_SUPPRESS_WARNING_POP
diff --git a/examples/all_features/test_output/decomposition.cpp.txt b/examples/all_features/test_output/decomposition.cpp.txt
index 7eba247c..5772c4ee 100644
--- a/examples/all_features/test_output/decomposition.cpp.txt
+++ b/examples/all_features/test_output/decomposition.cpp.txt
@@ -6,8 +6,11 @@ TEST CASE:  Move Only Type
 decomposition.cpp(0): ERROR: CHECK( genType(false) ) is NOT correct!
   values: CHECK( {?} )
 
+decomposition.cpp(0): ERROR: CHECK( a ) is NOT correct!
+  values: CHECK( {?} )
+
 ===============================================================================
-[doctest] test cases: 1 | 0 passed | 1 failed |
-[doctest] assertions: 2 | 1 passed | 1 failed |
+[doctest] test cases: 2 | 1 passed | 1 failed |
+[doctest] assertions: 4 | 2 passed | 2 failed |
 [doctest] Status: FAILURE!
 Program code.
diff --git a/examples/all_features/test_output/decomposition.cpp_junit.txt b/examples/all_features/test_output/decomposition.cpp_junit.txt
index 8ab87d46..4c51610d 100644
--- a/examples/all_features/test_output/decomposition.cpp_junit.txt
+++ b/examples/all_features/test_output/decomposition.cpp_junit.txt
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <testsuites>
-  <testsuite name="all_features" errors="0" failures="1" tests="2">
+  <testsuite name="all_features" errors="0" failures="2" tests="4">
     <testcase classname="decomposition.cpp" name="Move Only Type" status="run">
       <failure message="{?}" type="CHECK">
 decomposition.cpp(0):
@@ -8,7 +8,14 @@ CHECK( genType(false) ) is NOT correct!
   values: CHECK( {?} )
 
       </failure>
+      <failure message="{?}" type="CHECK">
+decomposition.cpp(0):
+CHECK( a ) is NOT correct!
+  values: CHECK( {?} )
+
+      </failure>
     </testcase>
+    <testcase classname="decomposition.cpp" name="Impl cast from non-const value" status="run"/>
   </testsuite>
 </testsuites>
 Program code.
diff --git a/examples/all_features/test_output/decomposition.cpp_xml.txt b/examples/all_features/test_output/decomposition.cpp_xml.txt
index 584f85be..e668a0f0 100644
--- a/examples/all_features/test_output/decomposition.cpp_xml.txt
+++ b/examples/all_features/test_output/decomposition.cpp_xml.txt
@@ -11,10 +11,21 @@
           {?}
         </Expanded>
       </Expression>
-      <OverallResultsAsserts successes="1" failures="1" test_case_success="false"/>
+      <Expression success="false" type="CHECK" filename="decomposition.cpp" line="0">
+        <Original>
+          a
+        </Original>
+        <Expanded>
+          {?}
+        </Expanded>
+      </Expression>
+      <OverallResultsAsserts successes="1" failures="2" test_case_success="false"/>
+    </TestCase>
+    <TestCase name="Impl cast from non-const value" filename="decomposition.cpp" line="0">
+      <OverallResultsAsserts successes="1" failures="0" test_case_success="true"/>
     </TestCase>
   </TestSuite>
-  <OverallResultsAsserts successes="1" failures="1"/>
-  <OverallResultsTestCases successes="0" failures="1"/>
+  <OverallResultsAsserts successes="2" failures="2"/>
+  <OverallResultsTestCases successes="1" failures="1"/>
 </doctest>
 Program code.
diff --git a/examples/all_features/test_output/filter_2.txt b/examples/all_features/test_output/filter_2.txt
index dd726cdf..a523bf4a 100644
--- a/examples/all_features/test_output/filter_2.txt
+++ b/examples/all_features/test_output/filter_2.txt
@@ -1,6 +1,6 @@
 [doctest] run with "--help" for options
 ===============================================================================
-[doctest] test cases: 0 | 0 passed | 0 failed | 103 skipped
+[doctest] test cases: 0 | 0 passed | 0 failed | 104 skipped
 [doctest] assertions: 0 | 0 passed | 0 failed |
 [doctest] Status: SUCCESS!
 Program code.
diff --git a/examples/all_features/test_output/filter_2_xml.txt b/examples/all_features/test_output/filter_2_xml.txt
index fd8a050a..648addd6 100644
--- a/examples/all_features/test_output/filter_2_xml.txt
+++ b/examples/all_features/test_output/filter_2_xml.txt
@@ -4,6 +4,7 @@
   <TestSuite>
     <TestCase name="  Scenario: vectors can be sized and resized" filename="subcases.cpp" line="0" skipped="true"/>
     <TestCase name="CHECK level of asserts fail the test case but don't abort it" filename="assertion_macros.cpp" line="0" skipped="true"/>
+    <TestCase name="Impl cast from non-const value" filename="decomposition.cpp" line="0" skipped="true"/>
     <TestCase name="Move Only Type" filename="decomposition.cpp" line="0" skipped="true"/>
     <TestCase name="Nested - related to https://github.com/doctest/doctest/issues/282" filename="subcases.cpp" line="0" skipped="true"/>
     <TestCase name="REQUIRE level of asserts fail and abort the test case - 1" filename="assertion_macros.cpp" line="0" skipped="true"/>
@@ -147,6 +148,6 @@
     <TestCase name="without a funny name:" filename="subcases.cpp" line="0" skipped="true"/>
   </TestSuite>
   <OverallResultsAsserts successes="0" failures="0"/>
-  <OverallResultsTestCases successes="0" failures="0" skipped="103"/>
+  <OverallResultsTestCases successes="0" failures="0" skipped="104"/>
 </doctest>
 Program code.
diff --git a/examples/all_features/test_output/no_multi_lane_atomics.txt b/examples/all_features/test_output/no_multi_lane_atomics.txt
index b95037bd..1ebce149 100644
--- a/examples/all_features/test_output/no_multi_lane_atomics.txt
+++ b/examples/all_features/test_output/no_multi_lane_atomics.txt
@@ -59,6 +59,9 @@ TEST CASE:  Move Only Type
 decomposition.cpp(0): ERROR: CHECK( genType(false) ) is NOT correct!
   values: CHECK( {?} )
 
+decomposition.cpp(0): ERROR: CHECK( a ) is NOT correct!
+  values: CHECK( {?} )
+
 ===============================================================================
 assertion_macros.cpp(0):
 TEST CASE:  REQUIRE level of asserts fail and abort the test case - 1
@@ -922,7 +925,7 @@ TEST CASE:  without a funny name:
 subcases.cpp(0): MESSAGE: Nooo
 
 ===============================================================================
-[doctest] test cases:  83 |  32 passed |  51 failed |
-[doctest] assertions: 228 | 109 passed | 119 failed |
+[doctest] test cases:  84 |  33 passed |  51 failed |
+[doctest] assertions: 230 | 110 passed | 120 failed |
 [doctest] Status: FAILURE!
 Program code.
diff --git a/examples/all_features/test_output/no_multithreading.txt b/examples/all_features/test_output/no_multithreading.txt
index b95037bd..1ebce149 100644
--- a/examples/all_features/test_output/no_multithreading.txt
+++ b/examples/all_features/test_output/no_multithreading.txt
@@ -59,6 +59,9 @@ TEST CASE:  Move Only Type
 decomposition.cpp(0): ERROR: CHECK( genType(false) ) is NOT correct!
   values: CHECK( {?} )
 
+decomposition.cpp(0): ERROR: CHECK( a ) is NOT correct!
+  values: CHECK( {?} )
+
 ===============================================================================
 assertion_macros.cpp(0):
 TEST CASE:  REQUIRE level of asserts fail and abort the test case - 1
@@ -922,7 +925,7 @@ TEST CASE:  without a funny name:
 subcases.cpp(0): MESSAGE: Nooo
 
 ===============================================================================
-[doctest] test cases:  83 |  32 passed |  51 failed |
-[doctest] assertions: 228 | 109 passed | 119 failed |
+[doctest] test cases:  84 |  33 passed |  51 failed |
+[doctest] assertions: 230 | 110 passed | 120 failed |
 [doctest] Status: FAILURE!
 Program code.
diff --git a/examples/all_features/test_output/std_headers.txt b/examples/all_features/test_output/std_headers.txt
index b95037bd..1ebce149 100644
--- a/examples/all_features/test_output/std_headers.txt
+++ b/examples/all_features/test_output/std_headers.txt
@@ -59,6 +59,9 @@ TEST CASE:  Move Only Type
 decomposition.cpp(0): ERROR: CHECK( genType(false) ) is NOT correct!
   values: CHECK( {?} )
 
+decomposition.cpp(0): ERROR: CHECK( a ) is NOT correct!
+  values: CHECK( {?} )
+
 ===============================================================================
 assertion_macros.cpp(0):
 TEST CASE:  REQUIRE level of asserts fail and abort the test case - 1
@@ -922,7 +925,7 @@ TEST CASE:  without a funny name:
 subcases.cpp(0): MESSAGE: Nooo
 
 ===============================================================================
-[doctest] test cases:  83 |  32 passed |  51 failed |
-[doctest] assertions: 228 | 109 passed | 119 failed |
+[doctest] test cases:  84 |  33 passed |  51 failed |
+[doctest] assertions: 230 | 110 passed | 120 failed |
 [doctest] Status: FAILURE!
 Program code.