Issue #1173 - Default behavior for catch-all blocks

test/plugin/ExceptionTest.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus 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 General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <gtest/gtest.h>
-
 #include "../../src/plugin/Exception.h"
-

[sergei, 2015/03/06 13:51:12]

These lines between #include are not necessary.

[Eric, 2015/03/06 17:29:59]

Done.

 #include <stdexcept>
 
 void AssignCurrentException(std::exception_ptr& e)

[sergei, 2015/03/06 13:51:12]

I don't think that we need this function

[Eric, 2015/03/06 17:29:59]

It's needed in order for the test 'CurrentExceptionWorksOutsideCatchHandler' to
be significant. The whole point is to call 'current_exception' in a stack frame
that's different from the one where it's caught.

 {
   e = std::current_exception();
 }
 
 /*
  * This test verifies that std::current_exception works during exception handling generally,
  *   not just in the immediate context of a 'catch' clause.
  */
 TEST(Exception, CurrentExceptionWorksOutsideCatchHandler)

[sergei, 2015/03/06 13:51:12]

What does it test? Standard C++ library?
Even more, we rethrow the exception in the body of `catch` but in this test it's
rethrown afterwards.
I would say we don't need this test.

[Eric, 2015/03/06 17:29:59]

It checks that the implementation of 'current_exception' is conformant. If it
doesn't conform in the way that this test validates, then the everything in this
module will not work correctly. It's important to check this separately in a
unit testing context, because if this fails, then there would be a need for a
rewrite to not use the present approach at all.

I don't except this test to fail in any ordinary circumstance, but because the
VC++ compiler has various compile-time options to change exception behavior,
it's important to keep the test around in case someone ever wants to fiddle with
those.

 {
   std::exception_ptr ep;
   const auto e = std::runtime_error("BJTCiRhkVAmvMg");
   try
   {
     throw e;
   }
   catch (...)
   {
     AssignCurrentException(ep);
     ASSERT_TRUE(ep);
     /*
      * You might think that the next set would be EXPECT_EQ betwwen the thrown exception and the original one.

[sergei, 2015/03/06 13:51:12]

This comment is not necessary.

[Eric, 2015/03/06 17:29:59]

I'm leaving it in because I got caught up with debugging a non-error as
described for too long. 

      * Such a test does not pass, nor is in necessary.
      * The throw statement above is throwing by value, so it makes a copy.
      */
   }
   try
   {
     rethrow_exception(ep);
     FAIL() << "Statement after 'rethrow_exception' executed.";
   }
   catch (std::runtime_error& ex)
@@ skipping 67 matching lines @@
 TEST(Exception, TrivialVoidRuntimeError)
 {
   TrivialVoid(std::runtime_error(""));
 }
 
 TEST(Exception, TrivialVoidSystemError)
 {
   TrivialVoid(std::system_error(std::error_code()));
 }
 
-enum ExceptionCode

[sergei, 2015/03/06 13:51:12]

It's not necessary here but I think it would be better to use `enum class`.

[Eric, 2015/03/06 17:29:59]

I specifically want an unscoped enumeration here. This is an isolated test file,
and using scoped names is just extra syntax that neither clarifies nor
disambiguates.

I changed it to an anonymous enum, which better reflects its unscoped nature. 

[sergei, 2015/03/31 14:30:51]

It's not an extra syntax, it helps to understand the logic here and prevent from
making mistakes. Especially in tests, where it's very important to properly
understand the logic.

In my changes I've made them scoped and there is no trouble with it. Even more,
it helped me to understand which classes should be touched to support custom
return type, which is one of the purposes of tests, to demonstrate how it can be
used.

[Eric, 2015/05/14 14:42:50]

I don't agree with you. It provides nothing extra.
I never said it couldn't work. I said it was unnecessary and extraneous.

If you'd like to change it, you can suggest your changes after the current
change set is committed.

[sergei, 2015/05/15 13:13:24]

If you does see it now it does not mean that it provides nothing extra.
Sure, I can, but why not to do it better at the first round?

+enum
 {
   InvalidCode = -1,
   UnknownCode = 1,
   ExceptionCode,
   LogicErrorCode,
   RuntimeErrorCode,
   SystemErrorCode
 };
 
 struct NullHandlersReturn
 {
-  typedef int return_t;

[sergei, 2015/03/31 14:30:51]

It's not related whether it's scoped or not (enumeration, not exception, right?)
because `typedef ExceptionCode return_t;` reflects better what we expect to have
as a return type here than int. See also
http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test....

[Eric, 2015/05/14 14:42:50]

Defining a typedef for the symbol 'return_t' reflects exactly that we are
expecting a return type. If that were not the expectation I would have just
declared them returning 'int' in the first place. It adds nothing to insist on a
second level of symbolic indirection by defining 'ExceptionCode'. It's _already_
defined as a return type. Indeed, it's less general to indicate that the return
type is supposed to be an exception code; it need not be at all (although that's
the way we'll be using it).

[sergei, 2015/05/15 13:13:24]

I really don't understand attempts to create less robust code by avoiding
assistance from the compiler and to write less self describing code. 

-  static return_t Unknown(int) { return UnknownCode; }
-  static return_t Exception(std::exception& ex, int) { return ExceptionCode; }
-  static return_t LogicError(std::logic_error& ex, int) { return LogicErrorCode; }
-  static return_t RuntimeError(std::runtime_error& ex, int) { return RuntimeErrorCode; }
-  static return_t SystemError(std::system_error& ex, int) { return SystemErrorCode; }
+  typedef int ReturnType;
+  static ReturnType Unknown(int) { return UnknownCode; }
+  static ReturnType Exception(std::exception& ex, int) { return ExceptionCode; }
+  static ReturnType LogicError(std::logic_error& ex, int) { return LogicErrorCode; }
+  static ReturnType RuntimeError(std::runtime_error& ex, int) { return RuntimeErrorCode; }
+  static ReturnType SystemError(std::system_error& ex, int) { return SystemErrorCode; }
 };
 
 template<class X>
 void TrivialReturn(int n, X x)
 {
   ASSERT_NO_THROW(
   {
     try
     {
       throw x;
@@ skipping 21 matching lines @@
 }
 
 TEST(Exception, TrivialReturnRuntimeError)
 {
   TrivialReturn(RuntimeErrorCode, std::runtime_error(""));
 }
 
 TEST(Exception, TrivialReturnSystemError)
 {
   TrivialReturn(SystemErrorCode, std::system_error(std::error_code()));
-}
-
-/*
- * The simple tests ensure that the flow of control arrives in the correct subhandler.
- */
-template<class X, class H>
-void SimpleVoid(X x, H h)

[sergei, 2015/03/06 13:51:12]

we don't need argument `H h` here as well as we don't need to instantiate
`Handler` below to call this method.

[Eric, 2015/03/06 17:29:59]

We need to have 'H' present in order to instantiate 'CatchAllVoid<H>'. That can
either happen through an explicit template argument or through type inference. I
chose type inference. It simplifies all the definitions and calls.

[sergei, 2015/03/31 14:30:51]

How does it simplify?
`SimpleVoid(std::runtime_error(""), Handler());` implies that we need an
instance of Handler, but we don't need it, it can be even not instantiable,
`SimpleVoid<Handler>(std::runtime_error(""));` is also quite straightforward. 

I've also changed it and there is no issue with it.

[Eric, 2015/05/14 14:42:50]

I picked a different syntax that the one you would have picked. I picked the one
that used type inference, which means that you instantiate a dummy variable. It
works.

Using an explicit template argument also works. I never said it wouldn't. I
prefer using a dummy instantiation.

[sergei, 2015/05/15 13:13:24]

So, how does it simplify?

-{
-  SimpleResult = 0;

[Eric, 2015/03/06 17:29:59]

Good suggestion.

Done.

-  SimpleExpected = rand();

[sergei, 2015/03/06 13:51:12]

`std::rand` and seeding is missed.

[Eric, 2015/03/06 17:29:59]

Added namespace scope.

If I were to provide a seed for this statement, it wouldn't be any different
than using a constant.

-  if (SimpleExpected == 0) { SimpleExpected = 1; }

[sergei, 2015/03/06 13:51:12]

Why do we need random numbers, I would say enum would be good here.

[sergei, 2015/03/31 14:30:51]

Sorry, I don't understand what kind of references are mentioned here. Randomness
is definitely overhead here.
I've tried to get rid of these global variables and it seems there is no any
trouble with it.

[Eric, 2015/05/14 14:42:50]

The word 'reference' can mean either its generic English meaning or a specific
C++ primitive type. I am using it here in its generic sense.
Of course you can get rid of the randomness. I don't think I claimed otherwise.
It's there, however, to substitute for local scope. If you get rid of the nonce,
you lose an element of the integrity check.

[sergei, 2015/05/15 13:13:24]

Such randomness here does not add any integrity check here, it only complicates
the logic and distracts from the main logic.

-  ASSERT_NO_THROW(
-  {
-    try
-    {
-      throw x;
-    }
-    catch (...)
-    {
-      CatchAllVoid<H>::Handler(0);
-    }
-  });
-  EXPECT_EQ(SimpleExpected, SimpleResult);
 }
 
 /*
  * VS 2012 supports thread_local semantics for POD only, not arbitrary types.
  * That's good enough for now.
  * Remove the definition when VS matures.
  */
 #define thread_local __declspec(thread)
 
 /*
  * The sub-handlers are purely static functions, 
  *   so getting them to return something unique for testing has possible race conditions.
  * We're using a thread-local variable as a return code from the simple handlers,
  *    which ensures that even a multi-threaded test runner will work here.
  */
 thread_local int SimpleResult;
 thread_local int SimpleExpected;

[sergei, 2015/03/06 13:51:12]

I don't like the idea to use global variables, and we can do it without them.

[Eric, 2015/03/06 17:29:59]

Sorry, they're needed here. I tried to get rid of them while writing these
tests, and the alternatives are worse.

The problem is that these handlers are 'public static', and essentially
equivalent to global functions in every way relevant to trying to achieve some
kind of local scope. As a substitute, I settled on globals + randomness as a
substitute. It's not perfect, but it does the real job, which is to exercise all
of the execution paths.

+
+/*
+ * The simple tests ensure that the flow of control arrives in the correct subhandler.
+ */
+template<class X, class H>
+void SimpleVoid(X x, H h)
+{
+  SimpleResult = 0;
+  SimpleExpected = std::rand();
+  if (SimpleExpected == 0) { SimpleExpected = 1; }
+  ASSERT_NO_THROW(
+  {
+    try
+    {
+      throw x;
+    }
+    catch (...)
+    {
+      CatchAllVoid<H>::Handler(0);
+    }
+  });
+  EXPECT_EQ(SimpleExpected, SimpleResult);
+}
 
 /*
  * The base handler class for the simple tests fails every execution path.
  * Each specific test redefines a single one of the handler functions that it uses.
  */
 class SimpleHandlersBase
 {
 public:
   static void Unknown(int) { FAIL() << "Unexpected exception of unknown type"; }
   static void Exception(std::exception& ex, int) { FAIL() << "Unexpected std::exception"; }
@@ skipping 73 matching lines @@
   });
   EXPECT_EQ(SimpleExpected, SimpleResult);
 }
 
 /*
  * The base handlers class fails every execution path.
  */
 class SimpleHandlersReturnBase
 {
 public:
-  typedef int return_t;
-  static return_t Unknown(int) { ADD_FAILURE() << "Unexpected exception of unknown type"; return InvalidCode; }
-  static return_t Exception(std::exception& ex, int) { ADD_FAILURE() << "Unexpected std::exception"; return InvalidCode; }
-  static return_t LogicError(std::logic_error& ex, int) { ADD_FAILURE() << "Unexpected std::logic_error"; return InvalidCode; }
-  static return_t RuntimeError(std::runtime_error& ex, int) { ADD_FAILURE() << "Unexpected std::runtime_error"; return InvalidCode; }
-  static return_t SystemError(std::system_error& ex, int) { ADD_FAILURE() << "Unexpected std::system_error"; return InvalidCode; }
+  typedef int ReturnType;
+  static ReturnType Unknown(int) { ADD_FAILURE() << "Unexpected exception of unknown type"; return InvalidCode; }
+  static ReturnType Exception(std::exception& ex, int) { ADD_FAILURE() << "Unexpected std::exception"; return InvalidCode; }
+  static ReturnType LogicError(std::logic_error& ex, int) { ADD_FAILURE() << "Unexpected std::logic_error"; return InvalidCode; }
+  static ReturnType RuntimeError(std::runtime_error& ex, int) { ADD_FAILURE() << "Unexpected std::runtime_error"; return InvalidCode; }
+  static ReturnType SystemError(std::system_error& ex, int) { ADD_FAILURE() << "Unexpected std::system_error"; return InvalidCode; }
 protected:
   static void Ping() { SimpleResult = SimpleExpected; }
 };
 
 TEST(Exception, SimpleReturnUnknown)
 {
   struct Handler :
     public SimpleHandlersReturnBase
   {
     static int Unknown(int) { Ping(); return UnknownCode; }
@@ skipping 34 matching lines @@
 TEST(Exception, SimpleReturnSystemError)
 {
   struct Handler :
     public SimpleHandlersReturnBase
   {
     static int SystemError(std::system_error& ex, int) { Ping(); return SystemErrorCode; }
   };
   SimpleReturn(SystemErrorCode, std::system_error(std::error_code()), Handler());
 }