Issue #1173 - Default behavior for catch-all blocks

src/plugin/PluginDebug.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 "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)
+    static void Unknown(const LogHandlersArguments& args)
+    {
+      CPluginDebug::Debug(DefaultLogMessage(args, "Unknown exception"));
+    }
+    static void Exception(const std::exception& ex, LogHandlersArguments& args)
     {
       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)
+        DefaultLogMessage(args, "std::exception"));
+    }
+    static void LogicError(const std::logic_error& ex, LogHandlersArguments& args)
     {
       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)
+        DefaultLogMessage(args, "std::logic_error"));
+    }
+    static void RuntimeError(const std::runtime_error& ex, LogHandlersArguments& args)
     {
       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)
+        DefaultLogMessage(args, "std::runtime_error"));
+    }
+    static void SystemError(const std::system_error& ex, LogHandlersArguments& args)
     {
       CPluginDebug::DebugSystemException(ex,
         PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
-        DefaultLogMessage(x, "std::std::system_error"));
+        DefaultLogMessage(args, "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)
@@ skipping 30 matching lines @@
 {
   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)
 {
 #ifdef USE_CONSOLE
   CONSOLE("%s", CT2A(text.GetString(), CP_UTF8));
 #endif
 
   if (CPluginSettings::HasInstance())
@@ skipping 188 matching lines @@
     srcTrunc = srcTrunc.Left(67) + L"..." + srcTrunc.Right(30);
   }
 
   CString blocking;
   blocking.Format(L"Ignored  %-12s  %s  %s", ToCString(type), domain.empty()? L"-" : ToCString(domain), srcTrunc);
 
   DebugResultLegacy(blocking);
 }
 
 #endif // ENABLE_DEBUG_RESULT_IGNORED