Issue #1173 - Default behavior for catch-all blocks

src/plugin/Exception.h

 /*
  * 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/>.
  */
 
 /** \file Exception.h -- Default behavior for catch-all exception handlers.
  */
 
 template<typename SubHandlers>
 struct CatchAllVoid
 {
   template<typename T>
-  static void Handler(T t=T())

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

Here and below spaces around '=' are missed and do we really need the default
argument value here?

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

Fixed.
In general, yes, since it allows instantiation without arguments if you don't
need them at all. I don't think any of the immediate code uses it, but I don't
consider that a very persuasive reason to remove the default.

With variadic template arguments, it wouldn't even be an issue. The handler
would take zero to many arguments, whatever its user wanted.

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

The aim of it is to pass user data to SubHandlers::Exception and it's not
necessary that user data type has the default constructor. Why to introduce this
requirement at so early stage?

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

It doesn't introduce a requirement unless you leave out the argument. That's a
consequence of how C++ instantiates templates.

Using a parameter here is a work-around for the lack of variadic templates in
the version of MS VC++ we're using, as was adequately documented elsewhere in
the patch set. The most analogous substitute is to allow 0 or 1 argument.

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

Without default argument value one cannot leave out the argument because
compiler will tell about it.
Default constructible argument and variadic templates are different things. For
example, it does not compile when T is a reference to an interface.

+  static void Handler(T t = T())

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

Let's rename T to UserData and t to userData.

   {
     try
     {
       std::rethrow_exception(std::current_exception());
     }
     catch (std::system_error& ex)

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

We don't need non const references, so it is better to add const modifier to all
exceptions, the argument that we will need to change the helper classes in tests
(btw, which are just introduced) should not work.
I've added it in my change, no troubles.

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

Using 'const' here is overly restrictive. It requires the 'SubHandlers' members
to all be declared 'const'. There's no need to enforce it here in the template.

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

Yes, it's more restrictive and requires `SubHandlers` members to have exception
argument as a const reference (it's different from your version). It saves us
from any kind of mistakes at the compile time. If we ever need non-const
reference to exception in some new `SubHandlers` then we can remove `const` here
and continue to use old `SubHandlers`.
Can someone tell when we need non-const reference to the exception?

     {
       SubHandlers::SystemError(ex, t);
     }
     catch (std::runtime_error& ex)
     {
       SubHandlers::RuntimeError(ex, t);
     }
     catch (std::logic_error& ex)
     {
       SubHandlers::LogicError(ex, t);
     }
     catch (std::exception& ex)
     {
       SubHandlers::Exception(ex, t);
     }
     catch (...)
     {
       SubHandlers::Unknown(t);
     }
   }
 };
 
-template<typename SubHandlers, typename ReturnType=SubHandlers::return_t>

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

missed spaces around '='

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

`return_t` is not according to our style

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

`typename` before `SubHandlers::return_t` is missed.

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

Done.

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

Done.

+template<typename SubHandlers, typename ReturnType = typename SubHandlers::ReturnType>
 struct CatchAllReturn
 {
   template<typename T>
-  static ReturnType Handler(T t=T())
+  static ReturnType Handler(T t = T())
   {
     try
     {
       std::rethrow_exception(std::current_exception());
       // Apparently VS 2012 does not realize that this function always throws.
       // Unless we have an explicit return statement, we get a spurious warning C4715 "not all control paths return a value".
       return ReturnType();
     }
     catch (std::system_error& ex)
     {
@@ skipping 10 matching lines @@
     catch (std::exception& ex)
     {
       return SubHandlers::Exception(ex, t);
     }
     catch (...)
     {
       return SubHandlers::Unknown(t);
     }
   }
 };