*_THROW(code, expected_exception) does not catch a pointer to an exception

[GoogleCodeExporter]

If you have:

  class Exception { };
  void foo(void) { throw new Exception(); }
  EXPECT_THROW(foo(), Exception);

The report will say that Exception was not thrown. This is because the
GTEST_TEST_THROW only catches "expected_exception const&" instead of also
considering "expected_exception const*".

It may be bad style to throw a pointer to an exception, but there's a lot
of legacy code out there that uses "throw new Exception()" instead of
simply "throw Exception()".

The following patch fixes this, and prevents a memory leak in the case of
pointers to exception.

====
//depot/agent/main/ThirdPartySource/gtest/1.1.0/include/gtest/internal/gtest-int
ernal.h#1
-
C:\depot\agent\main\ThirdPartySource\gtest\1.1.0\include\gtest\internal\gtest-in
ternal.h
====
@@ -728,6 +728,10 @@
     catch (expected_exception const&) { \
       gtest_caught_expected = true; \
     } \
+    catch (expected_exception const* expected_exception_const_pointer) { \
+      delete expected_exception_const_pointer;
+      gtest_caught_expected = true; \
+    } \
     catch (...) { \
       gtest_msg = "Expected: " #statement " throws an exception of type " \
                   #expected_exception ".\n  Actual: it throws a different " \

Original issue reported on code.google.com by halosta...@gmail.com on 7 Oct 2008 at 5:45

[GoogleCodeExporter]

I missed a backslash:

====
//depot/agent/main/ThirdPartySource/gtest/1.1.0/include/gtest/internal/gtest-int
ernal.h#1
-
C:\depot\agent\main\ThirdPartySource\gtest\1.1.0\include\gtest\internal\gtest-in
ternal.h
====
@@ -728,6 +728,10 @@
     catch (expected_exception const&) { \
       gtest_caught_expected = true; \
     } \
+    catch (expected_exception const* expected_exception_const_pointer) { \
+      delete expected_exception_const_pointer; \
+      gtest_caught_expected = true; \
+    } \
     catch (...) { \
       gtest_msg = "Expected: " #statement " throws an exception of type " \
                   #expected_exception ".\n  Actual: it throws a different " \

This is now correct and compiles.

Original comment by halosta...@gmail.com on 7 Oct 2008 at 5:53

[GoogleCodeExporter]

Thanks for the report and the patch.

However I'm not sure if the new behavior is more desirable.  It assumes the 
pointer
was created using new, which may not be true.  More importantly, it can be
misleading: when I see EXPECT_THROW(s, E) and the test passes, I may think that 
the
code is throwing an E, when it actually throws an E*.  I would want to be 
notified if
someone accidentally changes 'throw E()' to 'throw new E()' - I would not want 
the
test to continue to pass.

The philosophy behind EXPECT_THROW() and friends is that they provide a 
convenient
solution for the common case, not necessarily the general case.  You can always 
write
explicit try-catch if the macros aren't adequate for you.

Original comment by shiq...@gmail.com on 8 Oct 2008 at 9:25

• Changed state: WontFix
• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

The problem is that with the test as it stands, I can't do EXPECT_THROW(s, E*). 
I tried.

I'm not convinced, given the prevalence of "throw new E()" in our code (and 
much of
it is pre-2004 when I started), that the "common case" is "throw E()". There's 
enough
cases of "throw new E()" that try-catch becomes an imposition, not a solution.

I still think that there's a problem here when attempting to add tests to 
legacy code
that does this, when going through and changing everything from "throw new E()" 
to
"throw E()" isn't the appropriate answer.

Google Code Search suggests that there's ~13k instances of "throw new".
http://www.google.com/codesearch?q=%22throw+new%22+lang%3A%22C%2B%2B%22

Original comment by halosta...@gmail.com on 8 Oct 2008 at 9:54