Issue #1234 - Rework strings in debug facility

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
@@ skipping 33 matching lines @@
 CComAutoCriticalSection CPluginDebugLock::s_criticalSectionDebugLock;
 
 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);
 }
 
 #ifdef ENABLE_DEBUG_INFO
 
 void DebugLegacy(const CString& text, DWORD dwProcessId, DWORD dwThreadId)

[Oleksandr, 2015/02/27 07:50:15]

Isn't this changeset about converting CStrings to std strings in Debug
functions? Why do we need this legacy function?

[Eric, 2015/02/27 14:26:56]

It is, but I've split the work into two parts. First, in the present review,
I've changed just the API that these debug functions have, switching from
CString to std::wstring. Second, in work yet to be done, we'll rewrite the
internals of these debug functions to eliminate CString.

The trouble is that we don't have drop-in replacement functions for some of the
CString member functions. What that means is lots of rewriting and/or new
utility function. That would have increased the size of this change set too
much, so I split up this change into two parts.

 {
 #ifdef USE_CONSOLE
   CONSOLE("%s", CT2A(text.GetString(), CP_UTF8));
 #endif
 
   if (CPluginSettings::HasInstance())
   {
 #ifdef ENABLE_DEBUG_SPLIT_FILE
     CPluginSettings* settings = CPluginSettings::GetInstance();
 
@@ skipping 13 matching lines @@
     }
     if (dwThreadId == 0)
     {
       dwThreadId = ::GetCurrentThreadId();
     }
 
     CStringA processInfo;
     processInfo.Format("%4.4u.%4.4u - ", dwProcessId, dwThreadId);
 #endif
     SYSTEMTIME st;
     ::GetSystemTime(&st);

[Oleksandr, 2015/02/27 07:50:15]

Probably easier to replace this with std::localtime(std::time(nullptr)) and then
use std::strftime

[Eric, 2015/02/27 14:26:56]

Yes, we can do that when we rewrite this implementation.

 
     CStringA sysTime;
     sysTime.Format("%2.2d:%2.2d:%2.2d.%3.3d - ", st.wHour, st.wMinute, st.wSecond, st.wMilliseconds);
 
     CPluginDebugLock lock;
     if (lock.IsLocked())
     {
       std::ofstream debugFile;
 
 #ifdef ENABLE_DEBUG_SPLIT_FILE
       debugFile.open(GetDataPath(L"debug_" + processor + L".txt"), std::ios::app);
 #else
       debugFile.open(GetDataPath(L"debug.txt"), std::ios::app);
 #endif
       int pos = 0;
       CStringA line = text.Tokenize(L"\n\r", pos);

[Oleksandr, 2015/02/27 07:50:15]

Looks like just a place for wcstok_s again

[Eric, 2015/02/27 14:26:56]

Actually, we don't need to tokenize at all here. The only reason to tokenize at
all is to deal with multiple-line messages. What I did last summer was to add a
function that accepted arrays of strings; that worked just fine. I thought it
was clearer to show array brackets than newlines within strings to show what was
happening.

In any case, it's an issue for stage two, where we rewrite the internals of
these functions.

 
       while (pos >= 0)
       {
         debugFile.write(sysTime.GetBuffer(), sysTime.GetLength());
 #ifndef ENABLE_DEBUG_SPLIT_FILE
         debugFile.write(processInfo.GetBuffer(), processInfo.GetLength());
 #endif
         debugFile.write(line.GetBuffer(), line.GetLength());
         debugFile.write("\n", 1);
 
@@ skipping 22 matching lines @@
 void CPluginDebug::DebugException(const std::exception& ex)
 {
   auto error = std::string("!!! Exception:") + ex.what();
 #ifdef ENABLE_DEBUG_ERROR
   Debug(error);
 #endif
 
   DEBUG_SELFTEST("********************************************************************************\n" + error + "\n********************************************************************************")
 }
 
 void DebugErrorCodeLegacy(DWORD errorCode, const CString& error, DWORD dwProcessId, DWORD dwThreadId)

[Oleksandr, 2015/02/27 07:50:15]

Same here. Why do we need legacy functions after this changeset?

[Eric, 2015/02/27 14:26:56]

See above. Same reason.

 {
   CString errorCodeText;
   errorCodeText.Format(L"%u (0x%8.8x)", errorCode, errorCode);
 
   CString finalError = error + L". error=" + errorCodeText;
 
 #ifdef ENABLE_DEBUG_ERROR
   DebugLegacy(finalError, dwProcessId, dwThreadId);
 #endif
 
-  DEBUG_SELFTEST(L"********************************************************************************\n" + finalError + "\ n********************************************************************************")
+  DEBUG_SELFTEST(L"********************************************************************************\n" + finalError + "\n********************************************************************************")
 }
 
 void CPluginDebug::DebugErrorCode(DWORD errorCode, const std::string& error, DWORD processId, DWORD threadId)
 {
   DebugErrorCodeLegacy(errorCode, CString(error.c_str()), processId, threadId);
 }
 
 #endif
 
 // ============================================================================
@@ skipping 80 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