Issue #1173 - Default behavior for catch-all blocks

src/plugin/PluginDebug.cpp

Index: src/plugin/PluginDebug.cpp
===================================================================
--- a/src/plugin/PluginDebug.cpp
+++ b/src/plugin/PluginDebug.cpp
@@ -16,11 +16,84 @@
  */
 
 #include "PluginStdAfx.h"
-
 #include "PluginDebug.h"
+#include "Exception.h"
 #include "PluginMutex.h"
 #include "PluginSettings.h"
 
+#if (defined ENABLE_DEBUG_INFO)
+namespace {
+  // VS 2012 does not have support for variadic templates, which would eliminate the need for an argument class.

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

I don't think we need this comment.

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

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.

+  struct LogHandlersArguments
+  {
+    bool isEntryPoint;
+    std::string name;
+  };
+
+  std::string DefaultLogMessage(const LogHandlersArguments& x, const std::string &exceptionType)
+  {
+    std::string s = x.isEntryPoint ? "ENTRY POINT '" : "'";
+    s += x.name;
+    s += "' caught default ";
+    s += exceptionType;
+    return s;
+  }
+
+  struct LogHandlers
+  {
+    static void Unknown(LogHandlersArguments& x)

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

Would it be better to rename `x` to something else?
And why is not not const?

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

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

+    {
+      CPluginDebug::Debug(DefaultLogMessage(x, "Unknown exception"));
+    }
+    static void Exception(std::exception& ex, LogHandlersArguments& x)
+    {
+      CPluginDebug::DebugOrdinaryException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(x, "std::exception"));
+    }
+    static void LogicError(std::logic_error& ex, LogHandlersArguments& x)
+    {
+      CPluginDebug::DebugOrdinaryException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(x, "std::logic_error"));
+    }
+    static void RuntimeError(std::runtime_error& ex, LogHandlersArguments& x)
+    {
+      CPluginDebug::DebugOrdinaryException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(x, "std::runtime_error"));
+    }
+    static void SystemError(std::system_error& ex, LogHandlersArguments& x)
+    {
+      CPluginDebug::DebugSystemException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(x, "std::std::system_error"));
+    }
+  };
+}
+#endif
+
+void EntryPointExceptionDefault(const std::string& name)
+{
+#if (defined ENABLE_DEBUG_INFO)
+  // VS 2012 does not have support for brace initializer lists; otherwise this could be a single line

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

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.

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

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.

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

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.

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

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.

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

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

+  LogHandlersArguments x;
+  x.isEntryPoint = true;
+  x.name = name;
+  CatchAllVoid<LogHandlers>::Handler(x);
+#endif
+}
+
+void ExceptionDefault(const std::string& name)
+{
+#if (defined ENABLE_DEBUG_INFO)
+  LogHandlersArguments x;
+  x.isEntryPoint = false;
+  x.name = name;
+  CatchAllVoid<LogHandlers>::Handler(x);
+#endif
+}
+
 class CPluginDebugLock : public CPluginMutex
 {
 
@@ -45,10 +118,18 @@
 
 void CPluginDebug::DebugSystemException(const std::system_error& ex, int errorId, int errorSubid, const std::string& description)
 {
-  std::string message = description + ", " + ex.code().message() + ", " + ex.what();
-  DEBUG_ERROR_LOG(ex.code().value(), errorId, errorSubid, message);
+  std::string message = description + ", " + ex.code().message() + ", " + ex.what();
+  DEBUG_ERROR_LOG(ex.code().value(), errorId, errorSubid, message);
 }
 
+void CPluginDebug::DebugOrdinaryException(const std::exception& ex, int errorId, int errorSubid, const std::string& description)
+{
+  std::string message = description + ", " + ex.what();
+  DEBUG_ERROR_LOG(0, errorId, errorSubid, message);
+}
+
+

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

two additional lines

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

Done.

+
 #ifdef ENABLE_DEBUG_INFO
 
 void DebugLegacy(const CString& text, DWORD dwProcessId, DWORD dwThreadId)