Issue #1173 - Default behavior for catch-all blocks

Issue #1173 - Default behavior for catch-all blocks

Add template classes 'CatchAllVoid' and 'CatchAllReturn', which convert a
template class argument containing separate handlers for each exceptions type
into a single function that can be used as the entire body of a catch-all
block. The engine that makes it go are the standard library functions
'current_exception' and 'rethrow_exception'.

Add a function 'DefaultExceptionBehavior', defined with a set of trivial
subhandlers implemented in class 'NullHandlers'.

Sample usage:
  // ENTRY POINT
  try
  {
    // ... throw ...
  }
  catch (...)
  {
    DefaultExceptionBehavior();
  }

[Eric, 2015-02-25 22:05:22]

Review Notes

-- I didn't actually define a useful default behavior yet. Probably that should
be just to use the existing debug log facility. If we decide upon this approach,
it will be easy to write the appropriate behavior functions.

-- The default behavior function doesn't do anything in this version, but it's
well defined and can be used immediately.

-- The main reason I wrote these as template functions was to be able to test
the flow of control in all the handlers thoroughly. If we had to check the
behavior of a default handler by looking at files on disk, this facility would
be significantly harder to test.

-- Another benefit of a template implementation is that it's easy to write new
default exception behaviors. That's only useful if you have a set of functions
that should behave the same as each other, but perhaps slightly different than
the default. We have a situation like that now, however, in
http://codereview.adblockplus.org/4937147073167360/

-- There are both void and return versions of the catch-all handler. The return
version allows the statement 'return StandardBehavior();', so there's never a
need for more than one line in the catch-all block. Presumably the return value
will be something like E_FAIL.

-- There's a template I haven't written yet to convert a void-type set of
handlers into a return-type set of handlers with a fixed return value; that will
allow us to return the correct error code specific to the entry point. We'll
need something like that because entry points do not have consistent return
values for errors.

[Oleksandr, 2015-02-27 10:48:35]

This looks really cleaver, of course, however I still don't understand why we
keep focusing on this P4 issue. Even more, didn't we *just* agree that we should
add just simple try-catches first with whatever there has to be in catches
blocks and then optimize/generalize from there?
http://codereview.adblockplus.org/6032593782833152/#msg10

"One of the good lessons from the agile development community is that you should
(1) first get code working and (2) only then refactor the code"

This, IMO, is refactoring before the working code. Who knows, maybe we won't
need all these fancy classes at all.

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/adbl...
adblockplus.gyp:233: 'src/plugin/Exception.cpp',
What do we need an empty .cpp file for?

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/test...
File test/plugin/ExceptionTest.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/test...
test/plugin/ExceptionTest.cpp:36: const auto
e=std::runtime_error("BJTCiRhkVAmvMg");
Nit: Space before and after =

[Eric, 2015-02-27 15:17:40]

On 2015/02/27 10:48:35, Oleksandr wrote:
> Even more, didn't we *just* agree that we should
> add just simple try-catches first with whatever there has to be in catches
> blocks and then optimize/generalize from there?
> http://codereview.adblockplus.org/6032593782833152/#msg10

We did just agree with that, and this is completely compatible with that
decision. If you're going to write a try statement to prevent exception
propagation, you need a catch-all handler. This function allows you to do
something useful with that handler other than just "return E_FAIL;" or somesuch.
Such exceptions deserve investigation, and we really should at least be able to
log, during debugging and testing, the fact that we caught an exception that
wasn't handled explicitly. The whole point of this is to be able to put a single
function in a catch-all handler and be done with it. 

> This, IMO, is refactoring before the working code. Who knows, maybe we won't
> need all these fancy classes at all.

I don't consider it refactoring. It's a way of implementing a default behavior
with a single statement. This provides a way to handle exceptions _that are not
otherwise handled_. 

What I was objecting to before was a mechanism that interfered with writing
explicit catch handlers and encouraged a single behavior for all exceptions.
With this mechanism, you can always add an additional, specific handler to deal
with something explicitly. The try-statement is already there with a catch-all
handler, so all that's needed is to add a specific handler.

Whatever goes in the catch-all handler does not affect that in any way. If
you're dealing with some specific exception case in the function, the catch-all
handler doesn't help you at all. You can't do anything specific to the
'exception_ptr' that's available there. The only way to see its contents is to
throw it and examine it in a catch handler. And there's no point of doing that
if you want to handle the exception within the function body.

(Note: One of the anticipated use cases for 'exception_ptr' is to be able to
rethrow an exception in another thread. It might be reasonable to do that as a
default behavior for otherwise-unhandled exceptions (so that we move I/O out of
an execution path that needs to be fast), but if so, that's an issue orthogonal
to the present one.)

[Eric, 2015-02-27 15:36:50]

New patch set.

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/adbl...
adblockplus.gyp:233: 'src/plugin/Exception.cpp',
On 2015/02/27 10:48:36, Oleksandr wrote:
> What do we need an empty .cpp file for?

Oops. I had that in there before I realized that we could put everything in the
header.

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/test...
File test/plugin/ExceptionTest.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5629499534213120/test...
test/plugin/ExceptionTest.cpp:36: const auto
e=std::runtime_error("BJTCiRhkVAmvMg");
On 2015/02/27 10:48:36, Oleksandr wrote:
> Nit: Space before and after =

Done.

[Oleksandr, 2015-03-01 05:31:50]

> I don't consider it refactoring. It's a way of implementing a default behavior
> with a single statement. This provides a way to handle exceptions _that are
not
> otherwise handled_. 

My understanding of what we have agreed on is that we should not introduce code
that we are not sure we will use. This seems like exactly that code. Where will
we need to handle std::system_error, std::logic_error etc?

Wouldn't it be much simpler if DefaultExceptionBehavior was just a function that
logged exception and nothing more? I don't see how these 2 extra classes are
necessary right now. I am not even convinced we need this one unified
DefaultExceptionBehavior function right now at all. I'd vote to first start with
a simple log call in catch-all exception block and that's it.

Alternatively, maybe you might submit a patch-set where functionality built into
these classes would be justified.

[Eric, 2015-03-02 17:07:18]

New patch set.

Added a function that's suitable for default behavior in system entry points:
EntryPointExceptionDefault().

I added an argument to the handler functions so that
'EntryPointExceptionDefault' could accept function name argument to log where it
was called from. VS 2012 does not have variadic template arguments, which would
have been preferable (although VS 2013 does), so I made do with one fixed
argument, which is OK for now.

[Eric, 2015-03-02 17:21:43]

On 2015/03/01 05:31:50, Oleksandr wrote:
> My understanding of what we have agreed on is that we should not introduce
code
> that we are not sure we will use. This seems like exactly that code. Where
will
> we need to handle std::system_error, std::logic_error etc?

In general, everywhere. The goal of having proper exception handling to _enable_
writing code that throws exceptions. We have relatively little of that right
now, since throwing exceptions might cause crashing. We are already throwing
'system_error' for certain standard library functions. I have already been
wanting to throw 'logic_error' instead of masking with a plain error code.

> Wouldn't it be much simpler if DefaultExceptionBehavior was just a function
that
> logged exception and nothing more?

Well, that is entirely the point. The latest patch set implements such a
function.

> I don't see how these 2 extra classes are
> necessary right now. 

They're necessary because you cannot log any data from an exception caught with
(...) without using these classes (or their moral equivalent). There are no
conversions from 'std::exception_ptr' to typed instances, not even to
'std::exception'; it's a completely opaque class. (One caveat: it is convertible
to 'bool'.) The only way to find out what's inside an 'exception_ptr' is to
throw it and catch it again.

> I'd vote to first start with
> a simple log call in catch-all exception block and that's it.

If you do that, you won't be able to log the exception message that's in
'what()' for all the library exceptions. Without it, all you know is that
something happened and you get no other information.

[Oleksandr, 2015-03-04 05:07:31]

> They're necessary because you cannot log any data from an exception caught
with
> (...) without using these classes (or their moral equivalent)

I don't think the message from .what() will really give us any insight as to
what is going wrong in the code. I think we'd better log some unique message for
each catch(...) block if there's an error, so that we know *where* the error is.
That's why I don't see much sense in these classes. Even if you do insist on
using these, I'd like to extend them with an ability to add some context to the
exception. For example a variable that accepts value of __FUNCTION__, _FILE_,
_LINE_ etc would be nice.

[Eric, 2015-03-04 15:19:23]

On 2015/03/04 05:07:31, Oleksandr wrote:
> I don't think the message from .what() will really give us any insight as to
> what is going wrong in the code. 

Are you saying that you'd rather have less information than more information?
'what()' is certainly not perfect, but it's also much better than nothing. At
the very least it provides a clue about what's happening, rather than just
knowing that "something happened".

> I think we'd better log some unique message for
> each catch(...) block if there's an error, so that we know *where* the error
is.

That is exactly why I added arguments in patch set 3, so that we could do just
that. I put one in the handler for 'CPluginClass::Invoke' to illustrate how this
would work. (I know it's about to be superseded, but the same pattern can be
used in the replacement code.) Even if you need to supply such an argument
manually (see below), this provides enough information to uniquely locate a
catch block.

> Even if you do insist on
> using these, I'd like to extend them with an ability to add some context to
the
> exception. For example a variable that accepts value of __FUNCTION__, _FILE_,
> _LINE_ etc would be nice.

__FILE__ etc. are preprocessor definitions, so in order to use them consistently
you'd use them inside a wrapper macro definition. Whatever these macro
definitions are, they don't really affect what's already here, since the macro
can construct a single string argument. (Perhaps the macro definition uses a
helper function.)

In any case, whatever we do with macro wrappers, I'd prefer to do them in a
subsequent change set. This one is getting too large already; I would have
already preferred to have committed the debugging-specific code separately.

As what we can do with macro wrappers later, there are three kinds of things we
can do.

1) Wrap the call to an exception default function to uniquely identify where the
exception was _caught_. This is the case not only for entry point defaults, but
for other common defaults we may identify.

2) Wrap the constructor of exceptions in our own code to uniquely identify where
the exception was _thrown_. This information would end up in the 'what()' for
the exceptions we throw.

3) For exceptions that are thrown but not by us (e.g. i/o functions), we can do
something similar to (2). We can't change the 'what()' information in such an
external exception, but we can catch it and throw our own exception that does
have the location information. The standard library provides 'throw_with_nested'
for exactly this purpose. We would implement this by instantiating
'CatchAllVoid' with subhandlers that threw. We could then use such a default
handler in places where we did not want to handle the exception there but did
want to ensure we could find it later.

[Oleksandr, 2015-03-05 07:18:55]

Sorry, I have missed the last 2 patchsets in my previous comment. Patchset 3
does make this more useful.

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:58: void EntryPointExceptionDefault(std::string name)
I'd prefer 'DefaultExceptionHander' instead. This is not limited in any way to
only entry points.

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
File src/plugin/PluginDebug.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
src/plugin/PluginDebug.h:22: #define PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION
1
Shouldn't this should be in PluginErrorCodes.h?

[Eric, 2015-03-05 14:58:04]

Patch set 6 add ExceptionDefault() for exceptions caught outside of entry
points.

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:58: void EntryPointExceptionDefault(std::string name)
On 2015/03/05 07:18:55, Oleksandr wrote:
> I'd prefer 'DefaultExceptionHander' instead. This is not limited in any way to
> only entry points.

As a small point of language, there's no need to use "Handler" (or "Behavior",
my previous name) as a component of these names. It's completely clear from
context what they're doing, and the extra name doesn't add anything.

I had named this function with "EntryPoint" because entry points are special and
deserve special behavior. They sit at the root of the call stack and must not
rethrow. The policy I'd like to see is that exceptions should be handled
explicitly before they reach the catch-all exception handler of an entry point,
and thus that any exception that _is_ caught there is evidence of missing logic
elsewhere. At the start, this could be as simple as a magic string in the debug
log, but there should be some distinction. Using a separate function also
provides a distinct location to place a breakpoint.

To address your point, I'll add an unadorned 'ExceptionDefault' that can be used
elsewhere. Its log message won't have a special logging annotation. Personally,
I think that the default for non-root exceptions should be to rethrow, in
addition to logging, rather than only logging and thus suppressing the
exception. I've not done that in the present change set, since it's slightly
premature. We can simply add a 'throw' statement with no operand after
'ExceptionDefault' for now to get this behavior.

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
File src/plugin/PluginDebug.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5733935958982656/src/...
src/plugin/PluginDebug.h:22: #define PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION
1
On 2015/03/05 07:18:55, Oleksandr wrote:
> Shouldn't this should be in PluginErrorCodes.h?

Well, I would prefer to eliminate PluginErrorCodes.h and put all the definitions
here; that headers always need to be used together with this one. If you don't
mind, we can let that change ride along with this one. Alternately, I'll just
move it. Let me know.

[Oleksandr, 2015-03-06 04:02:52]

> Well, I would prefer to eliminate PluginErrorCodes.h and put all the
definitions
> here; that headers always need to be used together with this one. If you don't
> mind, we can let that change ride along with this one. Alternately, I'll just
> move it. Let me know.

Persoanlly I'll prefer to keep them all in one place, and if we do remove
PluginErrorCodes.h (why? I don't see many benefits of that) we can move them all
here then.

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

First of all, it looks really nice!

However, I still hope will replace
```
try
{
  useful code
}
catch(...)
{
  HandleEntryPointException();
}
```
by something like
```
ENTRY_POINT_TRY
useful code
ENTRY_POINT_CATCH(HandleEntryPointException)
```

Could you rebase it, there are conflicts.

A couple of comments which are related to many places in the code:
- Why are all references to exception not const references? We don't call any
non-const method.
- `ExceptionHandler::SystemError`, `ExceptionHandler::RuntimeError`,
`ExceptionHandler::Exception`, it would make it much better to have only one
name (e.g., `ExceptionHandler::Exception`) than have a specific name for each
exception type.

On 2015/03/06 04:02:52, Oleksandr wrote:
> > Well, I would prefer to eliminate PluginErrorCodes.h and put all the
> definitions
> > here; that headers always need to be used together with this one. If you
don't
> > mind, we can let that change ride along with this one. Alternately, I'll
just
> > move it. Let me know.
> 
> Persoanlly I'll prefer to keep them all in one place, and if we do remove
> PluginErrorCodes.h (why? I don't see many benefits of that) we can move them
all
> here then.
I also would like to keep them all in one place.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/adbl...
adblockplus.gyp:232: 'src/plugin/Exception.h',
Do we need also to add it to addon project?

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:25: static void Handler(T t=T())
Here and below spaces around '=' are missed and do we really need the default
argument value here?

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:54: template<typename SubHandlers, typename
ReturnType=SubHandlers::return_t>
missed spaces around '='

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:54: template<typename SubHandlers, typename
ReturnType=SubHandlers::return_t>
`return_t` is not according to our style

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:54: template<typename SubHandlers, typename
ReturnType=SubHandlers::return_t>
`typename` before `SubHandlers::return_t` is missed.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginClass.cpp:772:
EntryPointExceptionDefault("CPluginClass::Invoke");
I expected such methods to determine what should be returned, thus I expected to
see something like
`return EntryPointExceptionToHRESULT("CPluginClass::Invoke", E_FAIL);`

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:26: // VS 2012 does not have support for variadic
templates, which would eliminate the need for an argument class.
I don't think we need this comment.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:44: static void Unknown(LogHandlersArguments& x)
Would it be better to rename `x` to something else?
And why is not not const?

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:79: // VS 2012 does not have support for brace
initializer lists; otherwise this could be a single line
I would remove this comment because it's useless here.
For the present we can add the constructor to `LogHandlersArguments` and do it
in one line, but the advantage of the current code is that it's immediately
clear what is specified, with the constructor or with any initializer list we
lose `isEntryPoint` and `name`.
For me the current variant as well as the variant with the constructor are fine.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:131: 
two additional lines

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
File test/plugin/ExceptionTest.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:21: 
These lines between #include are not necessary.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:24: void
AssignCurrentException(std::exception_ptr& e)
I don't think that we need this function

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:33: TEST(Exception,
CurrentExceptionWorksOutsideCatchHandler)
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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:46: * You might think that the next set would be
EXPECT_EQ betwwen the thrown exception and the original one.
This comment is not necessary.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:134: enum ExceptionCode
It's not necessary here but I think it would be better to use `enum class`.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:146: typedef int return_t;
As I understand it should be `typedef ExceptionCode return_t;`

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:199: void SimpleVoid(X x, H h)
we don't need argument `H h` here as well as we don't need to instantiate
`Handler` below to call this method.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:201: SimpleResult = 0;
Could you define `SimpleResult` and `SimpleExpected` before this function.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:202: SimpleExpected = rand();
`std::rand` and seeding is missed.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:203: if (SimpleExpected == 0) { SimpleExpected =
1; }
Why do we need random numbers, I would say enum would be good here.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:232: thread_local int SimpleExpected;
I don't like the idea to use global variables, and we can do it without them.

[Eric, 2015-03-06 17:29:58]

New patch set addresses @sergei's comments.  Current patch set is based of
public tip.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/adbl...
adblockplus.gyp:232: 'src/plugin/Exception.h',
On 2015/03/06 13:51:12, sergei wrote:
> Do we need also to add it to addon project?

This exception-handling pattern is also applicable to the engine, so under that
argument, it belongs in 'shared'. But I would like to use this header in the
custom actions for the installer, as well. It really belongs in the 'commmon'
library, but that change hasn't been committed yet. For now, then I'll keep it
here and move it into 'common' when that's ready.

The review for the 'common' project:
    http://codereview.adblockplus.org/6216090891845632/
I'm awaiting one approval for that review before pushing the code. That approval
is yours, @sergei.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:25: static void Handler(T t=T())
On 2015/03/06 13:51:12, sergei wrote:
> Here and below spaces around '=' are missed 

Fixed.

> do we really need the default
> argument value here?

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:54: template<typename SubHandlers, typename
ReturnType=SubHandlers::return_t>
On 2015/03/06 13:51:12, sergei wrote:
> missed spaces around '='

Done.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:54: template<typename SubHandlers, typename
ReturnType=SubHandlers::return_t>
On 2015/03/06 13:51:12, sergei wrote:
> `return_t` is not according to our style

Done.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginClass.cpp:772:
EntryPointExceptionDefault("CPluginClass::Invoke");
On 2015/03/06 13:51:12, sergei wrote:
> I expected such methods to determine what should be returned, thus I expected
to
> see something like
> `return EntryPointExceptionToHRESULT("CPluginClass::Invoke", E_FAIL);`

For the generic version for arbitrary entry point handlers, I would not pass an
argument to a function just so that I could get it back as the return value.
Such an argument wouldn't be used in any essential way inside the function and
is best left out.

On the other hand, for entry points that are part of set, such as those for the
pieces of IDispatch, one could put the return value in the definition and use it
thus:
    catch (...)
    {
      return
DispatchEntryPointExceptionDefault("CPluginClass::OnDocumentComplete");
    }

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:26: // VS 2012 does not have support for variadic
templates, which would eliminate the need for an argument class.
On 2015/03/06 13:51:12, sergei wrote:
> I don't think we need this comment.

This comment and the one like it below use the marker "VS 2012" to indicate code
I want to rewrite when we upgrade to VS 2013 (or later. Had we had it already, I
would have written it differently. Using the marker will allow me to use 'grep'
to find them all later.

This kind of thing is too lightweight to be worth an issue in the Trac.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:44: static void Unknown(LogHandlersArguments& x)
On 2015/03/06 13:51:12, sergei wrote:
> Would it be better to rename `x` to something else?
> And why is not not const?

Added 'const'. Renamed 'x' to 'args'.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:79: // VS 2012 does not have support for brace
initializer lists; otherwise this could be a single line
On 2015/03/06 13:51:12, sergei wrote:
> the advantage of the current code is that it's immediately
> clear what is specified, with the constructor or with any initializer list we
> lose `isEntryPoint` and `name`.

I agree. The virtue of naming the structure elements outweighed concision here
for me. When we've got variadic template arguments, I'll pass these argument in
directly and eliminate the arguments-structure.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:131: 
On 2015/03/06 13:51:12, sergei wrote:
> two additional lines

Done.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
File test/plugin/ExceptionTest.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:21: 
On 2015/03/06 13:51:12, sergei wrote:
> These lines between #include are not necessary.

Done.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:24: void
AssignCurrentException(std::exception_ptr& e)
On 2015/03/06 13:51:12, sergei wrote:
> I don't think that we need this function

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:33: TEST(Exception,
CurrentExceptionWorksOutsideCatchHandler)
On 2015/03/06 13:51:12, sergei wrote:
> What does it test? Standard C++ library?

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:46: * You might think that the next set would be
EXPECT_EQ betwwen the thrown exception and the original one.
On 2015/03/06 13:51:12, sergei wrote:
> This comment is not necessary.

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

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:134: enum ExceptionCode
On 2015/03/06 13:51:12, sergei wrote:
> It's not necessary here but I think it would be better to use `enum class`.

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:146: typedef int return_t;
On 2015/03/06 13:51:12, sergei wrote:
> As I understand it should be `typedef ExceptionCode return_t;`

That would be true if 'ExceptionCode' were a scoped exception.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:199: void SimpleVoid(X x, H h)
On 2015/03/06 13:51:12, sergei wrote:
> we don't need argument `H h` here as well as we don't need to instantiate
> `Handler` below to call this method.

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:201: SimpleResult = 0;
On 2015/03/06 13:51:12, sergei wrote:
> Could you define `SimpleResult` and `SimpleExpected` before this function.

Good suggestion.

Done.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:202: SimpleExpected = rand();
On 2015/03/06 13:51:12, sergei wrote:
> `std::rand` and seeding is missed.

Added namespace scope.

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

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:203: if (SimpleExpected == 0) { SimpleExpected =
1; }
On 2015/03/06 13:51:12, sergei wrote:
> Why do we need random numbers, I would say enum would be good here.

It makes the test slightly more robust in maintenance. You have to actually
reference 'SimpleExpected' by name to get the right value, and that's makes for
an obvious cheat.

If there were a good way to localize the scope of the test, we could just use
local variables and a constant. Using a random number much the same effect as
having local scope.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:232: thread_local int SimpleExpected;
On 2015/03/06 13:51:12, sergei wrote:
> I don't like the idea to use global variables, and we can do it without them.

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.

[Eric, 2015-03-06 17:42:34]

On 2015/03/06 13:51:12, sergei wrote:
> First of all, it looks really nice!

Thank you.

> something like
> ```
> ENTRY_POINT_TRY
> useful code
> ENTRY_POINT_CATCH(HandleEntryPointException)
> ```

Thankfully, we don't need to discuss that now. (Read: we don't need to bicker
over that now.)

> A couple of comments which are related to many places in the code:
> - Why are all references to exception not const references? We don't call any
> non-const method.

Point them out with in-line comments. I see I need to add them in LogHandlers.
In the unit tests they wouldn't hurt but also wouldn't add much.

> - `ExceptionHandler::SystemError`, `ExceptionHandler::RuntimeError`,
> `ExceptionHandler::Exception`, it would make it much better to have only one
> name (e.g., `ExceptionHandler::Exception`) than have a specific name for each
> exception type.

You need some kind of unique name for each exception type you handle, otherwise
you can't disambiguate them otherwise. It seemed easiest just to write an
explicit identifier for them. An alternative would be to use template
specialization, but I don't think it would be any clearer, and it would be
definitely be more verbose.

Re: PluginErrorCodes.h
> I also would like to keep them all in one place.

I forgot to do this in patch set 6.

[Eric, 2015-03-06 18:12:57]

New patch set 7.

I rolled in all the definitions in PluginErrorCode.h that were actually used
into PluginDebug.h. The only place these definitions are actually used is as
arguments (ultimately) to 'DebugErrorCode'. There was never a good reason to
have these defined in separate files.

I added a few 'const' declarations per @sergei's comments.

[Oleksandr, 2015-03-19 14:02:43]

One nit. LGTM otherwise.

http://codereview.adblockplus.org/5137721374801920/diff/5730192894984192/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5730192894984192/src/...
src/plugin/Exception.h:58: static ReturnType Handler(T t=T())
Spaces before and after '='

[Eric, 2015-03-20 09:56:36]

New patch set 7 is rebased, including the extensive modifications to
'CPluginClass'. The nit fix is also in this patch set. I had to manually fix up
the rebase, so please double-check this one.

Separately, patch set 8 replaces the entry point call for the old
'CPluginClass::Invoke()' (now implemented in an ATL base class) with calls in
each of the event functions. It's the morally equivalent code as before.

http://codereview.adblockplus.org/5137721374801920/diff/5730192894984192/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5730192894984192/src/...
src/plugin/Exception.h:58: static ReturnType Handler(T t=T())
On 2015/03/19 14:02:43, Oleksandr wrote:
> Spaces before and after '='

Done.

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

I've played a bit with the code in tests, one can find it here
http://codereview.adblockplus.org/5743529531801600/ . The changes address my
comments, I guess it better demonstrates the good sides and that there is no
additional complexity than if I described it in text.

> > - `ExceptionHandler::SystemError`, `ExceptionHandler::RuntimeError`,
> > `ExceptionHandler::Exception`, it would make it much better to have only one
> > name (e.g., `ExceptionHandler::Exception`) than have a specific name for
each
> > exception type.
> 
> You need some kind of unique name for each exception type you handle,
otherwise
> you can't disambiguate them otherwise. It seemed easiest just to write an
> explicit identifier for them. An alternative would be to use template
> specialization, but I don't think it would be any clearer, and it would be
> definitely be more verbose.

It's not necessary to have unique names, otherwise it won't be possible to
implement visitor pattern in C++. There is already an example in the tests when
in `struct NullHandlers` we don't need to implement all exception methods,
`static void Exception(const std::exception&,int) {}` is enough.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:25: static void Handler(T t=T())
On 2015/03/06 17:29:59, Eric wrote:
> On 2015/03/06 13:51:12, sergei wrote:
> > Here and below spaces around '=' are missed 
> 
> Fixed.
> 
> > do we really need the default
> > argument value here?
> 
> 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.

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?

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:79: // VS 2012 does not have support for brace
initializer lists; otherwise this could be a single line
On 2015/03/06 17:29:59, Eric wrote:
> On 2015/03/06 13:51:12, sergei wrote:
> > the advantage of the current code is that it's immediately
> > clear what is specified, with the constructor or with any initializer list
we
> > lose `isEntryPoint` and `name`.
> 
> I agree. The virtue of naming the structure elements outweighed concision here
> for me. When we've got variadic template arguments, I'll pass these argument
in
> directly and eliminate the arguments-structure.

Sorry, it's not clear for me why you want to have variadic template  arguments
here. There is no need of some additional flexibility now but after adding them
the complexity of the code will be higher.

Anyway, I would remove these comments regarding variadic templates because they
don't explain anything specific in the logic here.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
File test/plugin/ExceptionTest.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:134: enum ExceptionCode
On 2015/03/06 17:29:59, Eric wrote:
> On 2015/03/06 13:51:12, sergei wrote:
> > It's not necessary here but I think it would be better to use `enum class`.
> 
> 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. 

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:146: typedef int return_t;
On 2015/03/06 17:29:59, Eric wrote:
> On 2015/03/06 13:51:12, sergei wrote:
> > As I understand it should be `typedef ExceptionCode return_t;`
> 
> That would be true if 'ExceptionCode' were a scoped exception.
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....

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:199: void SimpleVoid(X x, H h)
On 2015/03/06 17:29:59, Eric wrote:
> On 2015/03/06 13:51:12, sergei wrote:
> > we don't need argument `H h` here as well as we don't need to instantiate
> > `Handler` below to call this method.
> 
> 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.

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:203: if (SimpleExpected == 0) { SimpleExpected =
1; }
On 2015/03/06 17:29:59, Eric wrote:
> On 2015/03/06 13:51:12, sergei wrote:
> > Why do we need random numbers, I would say enum would be good here.
> 
> It makes the test slightly more robust in maintenance. You have to actually
> reference 'SimpleExpected' by name to get the right value, and that's makes
for
> an obvious cheat.
> 
> If there were a good way to localize the scope of the test, we could just use
> local variables and a constant. Using a random number much the same effect as
> having local scope.

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.

http://codereview.adblockplus.org/5137721374801920/diff/5635415851663360/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5635415851663360/src/...
src/plugin/Exception.h:25: static void Handler(T t = T())
Let's rename T to UserData and t to userData.

http://codereview.adblockplus.org/5137721374801920/diff/5635415851663360/src/...
src/plugin/Exception.h:31: catch (std::system_error& ex)
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:38:51]

On 2015/03/31 14:30:51, sergei wrote:
> It's not necessary to have unique names, otherwise it won't be possible to
> implement visitor pattern in C++.

We don't need to use the visitor pattern. 'std::exception::what()' is already
virtual. The only reason that there's a handler for more than just
'std::exception' is that I don't trust the MS VC++ standard library
implementation to be adequately informative. If it proves that they are, we can
eliminate some of the handlers here.

If we need extra information in an exception, the right way to do it is with our
own exception class derived from either 'runtime_error' or 'logic_error'.
Overriding 'what()' allows us to put whatever text we want.

From an engineering practice perspective, changing the code to add support for
any pattern is premature, because we're not even using these classes yet.
Changing them to anticipate behavior we're not using is work we shouldn't be
doing now. If we need it later, we can add it later.

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

I made no code changes based on the latest round of comments. No new patch set.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:25: static void Handler(T t=T())
On 2015/03/31 14:30:51, sergei wrote:
> 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?

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:79: // VS 2012 does not have support for brace
initializer lists; otherwise this could be a single line
On 2015/03/31 14:30:51, sergei wrote:
> Anyway, I would remove these comments regarding variadic templates because
they
> don't explain anything specific in the logic here.

The comments about variadic templates stay. It's indicating how to change the
code when we upgrade compiler versions.

The code will be simpler, not more complicated, with variadic templates, because
all the arguments become pass-through arguments.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
File test/plugin/ExceptionTest.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:134: enum ExceptionCode
On 2015/03/31 14:30:51, sergei wrote:
> it helps to understand the logic here 

I don't agree with you. It provides nothing extra.

> In my changes I've made them scoped and there is no trouble with it.

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:146: typedef int return_t;
On 2015/03/31 14:30:51, sergei wrote:
> `typedef ExceptionCode return_t;` reflects better what we expect to have
> as a return type here than int. 

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).

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:199: void SimpleVoid(X x, H h)
On 2015/03/31 14:30:51, sergei wrote:
> I've also changed it and there is no issue with it.

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.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:203: if (SimpleExpected == 0) { SimpleExpected =
1; }
On 2015/03/31 14:30:51, sergei wrote:
> Sorry, I don't understand what kind of references are mentioned here. 

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.

> 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.

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.

http://codereview.adblockplus.org/5137721374801920/diff/5635415851663360/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5635415851663360/src/...
src/plugin/Exception.h:31: catch (std::system_error& ex)
On 2015/03/31 14:30:51, sergei wrote:
> We don't need non const references, so it is better to add const modifier to
all
> exceptions

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]

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/Exception.h:25: static void Handler(T t=T())
On 2015/05/14 14:42:50, Eric wrote:
> It doesn't introduce a requirement unless you leave out the argument.
Without default argument value one cannot leave out the argument because
compiler will tell about it.

> On 2015/03/31 14:30:51, sergei wrote:
> 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.
> 

Default constructible argument and variadic templates are different things. For
example, it does not compile when T is a reference to an interface.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/src/...
src/plugin/PluginDebug.cpp:79: // VS 2012 does not have support for brace
initializer lists; otherwise this could be a single line
On 2015/05/14 14:42:50, Eric wrote:
> On 2015/03/31 14:30:51, sergei wrote:
> > Anyway, I would remove these comments regarding variadic templates because
> they
> > don't explain anything specific in the logic here.
> 
> The comments about variadic templates stay. It's indicating how to change the
> code when we upgrade compiler versions.
> 
> The code will be simpler, not more complicated, with variadic templates,
because
> all the arguments become pass-through arguments.

There are another more reliable technics how to find all places which should be
changed, this comment looks redundant here.

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
File test/plugin/ExceptionTest.cpp (right):

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:134: enum ExceptionCode
On 2015/05/14 14:42:50, Eric wrote:
> On 2015/03/31 14:30:51, sergei wrote:
> > it helps to understand the logic here 
> 
> I don't agree with you. It provides nothing extra.
If you does see it now it does not mean that it provides nothing extra.

> 
> > In my changes I've made them scoped and there is no trouble with it.
> 
> 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.
Sure, I can, but why not to do it better at the first round?

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:146: typedef int return_t;
On 2015/05/14 14:42:50, Eric wrote:
> On 2015/03/31 14:30:51, sergei wrote:
> > `typedef ExceptionCode return_t;` reflects better what we expect to have
> > as a return type here than int. 
> 
> 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).

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

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:199: void SimpleVoid(X x, H h)
On 2015/05/14 14:42:50, Eric wrote:
> On 2015/03/31 14:30:51, sergei wrote:
> > I've also changed it and there is no issue with it.
> 
> 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.
> 

So, how does it simplify?

http://codereview.adblockplus.org/5137721374801920/diff/5759778777202688/test...
test/plugin/ExceptionTest.cpp:203: if (SimpleExpected == 0) { SimpleExpected =
1; }
On 2015/05/14 14:42:50, Eric wrote:
> On 2015/03/31 14:30:51, sergei wrote:
> > Sorry, I don't understand what kind of references are mentioned here. 
> 
> 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.
> 
> > 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.
> 
> 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.
> 

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

http://codereview.adblockplus.org/5137721374801920/diff/5635415851663360/src/...
File src/plugin/Exception.h (right):

http://codereview.adblockplus.org/5137721374801920/diff/5635415851663360/src/...
src/plugin/Exception.h:31: catch (std::system_error& ex)
On 2015/05/14 14:42:50, Eric wrote:
> On 2015/03/31 14:30:51, sergei wrote:
> > We don't need non const references, so it is better to add const modifier to
> all
> > exceptions
> 
> 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.

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?