Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(427)

Issue 5747779603267584: Issue #1234 - Rework strings in debug facility (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by Eric
Modified:
4 years, 10 months ago
Reviewers:
Oleksandr, sergei
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue #1234 - Rework strings in debug facility Remove CString from all interfaces within PluginDebug.h and for the debugging parts of PluginClientBase.h. Reworked PluginDebug.cpp, using the old implementations and conversions for the new functions. This isolates CString for this facility to occur only within the implementation. Remove PluginDebugMacros.h and unused definitions. Many of the definitions in file PluginDebugMacros.h were simply providing default definitions for 'DEBUG_*' macros that were then redefined, for debug builds only, within Config.h. Move all the relevant defaults into Config.h, changing the #undef/#define pattern with one of always defining the macro with #if/#else. Add DEBUG_SYSTEM_EXCEPTION, which refactors three occurrences of identical code to a single function. Add DEBUG_EXCEPTION, which refactors two occurrences. Remove some never-referenced macro definitions. Removed DebugClear() and DebugResultClear(), which were never called.

Patch Set 1 : #

Total comments: 6

Patch Set 2 : Fixed spaces #

Total comments: 10

Patch Set 3 : rebase #

Total comments: 2

Patch Set 4 : typo fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -247 lines) Patch
M adblockplus.gyp View 1 chunk +0 lines, -1 line 0 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M src/plugin/AdblockPlusDomTraverser.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M src/plugin/Config.h View 1 2 2 chunks +24 lines, -68 lines 0 comments Download
M src/plugin/Plugin.cpp View 1 chunk +6 lines, -6 lines 0 comments Download
M src/plugin/PluginClass.cpp View 1 2 4 chunks +11 lines, -9 lines 0 comments Download
M src/plugin/PluginClientBase.h View 1 2 3 chunks +5 lines, -5 lines 0 comments Download
M src/plugin/PluginClientBase.cpp View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M src/plugin/PluginDebug.h View 1 chunk +10 lines, -11 lines 0 comments Download
M src/plugin/PluginDebug.cpp View 1 2 3 11 chunks +39 lines, -42 lines 0 comments Download
R src/plugin/PluginDebugMacros.h View 1 chunk +0 lines, -64 lines 0 comments Download
M src/plugin/PluginFilter.cpp View 7 chunks +9 lines, -9 lines 0 comments Download
M src/plugin/PluginStdAfx.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/plugin/PluginTabBase.cpp View 1 2 4 chunks +25 lines, -24 lines 0 comments Download
M src/plugin/PluginWbPassThrough.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 15
Eric
Some notes on out-of-scope changes that might be combined here, but are not: -- I ...
4 years, 10 months ago (2015-02-24 15:23:30 UTC) #1
sergei
Great! http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h File src/plugin/Config.h (right): http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h#newcode96 src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description) CPluginClientBase::PostPluginError(err, id, subid, ...
4 years, 10 months ago (2015-02-25 13:53:31 UTC) #2
Eric
http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h File src/plugin/Config.h (right): http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h#newcode96 src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description) CPluginClientBase::PostPluginError(err, id, subid, description); ...
4 years, 10 months ago (2015-02-25 15:24:46 UTC) #3
sergei
LGTM http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h File src/plugin/Config.h (right): http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h#newcode96 src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description) CPluginClientBase::PostPluginError(err, id, subid, ...
4 years, 10 months ago (2015-02-25 15:51:23 UTC) #4
Eric
http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h File src/plugin/Config.h (right): http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/plugin/Config.h#newcode96 src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description) CPluginClientBase::PostPluginError(err, id, subid, description); ...
4 years, 10 months ago (2015-02-25 16:13:59 UTC) #5
Oleksandr
This patch-set should have really been split into two, IMO. One for actual CString removal ...
4 years, 10 months ago (2015-02-27 07:50:15 UTC) #6
Eric
http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/plugin/PluginDebug.cpp File src/plugin/PluginDebug.cpp (right): http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/plugin/PluginDebug.cpp#newcode54 src/plugin/PluginDebug.cpp:54: void DebugLegacy(const CString& text, DWORD dwProcessId, DWORD dwThreadId) On ...
4 years, 10 months ago (2015-02-27 14:26:56 UTC) #7
Eric
On 2015/02/27 07:50:15, Oleksandr wrote: > This patch-set should have really been split into two, ...
4 years, 10 months ago (2015-02-27 14:40:31 UTC) #8
Oleksandr
> Well, it is related, since I was having trouble auditing the code before I ...
4 years, 10 months ago (2015-03-03 20:49:33 UTC) #9
Eric
On 2015/03/03 20:49:33, Oleksandr wrote: > I still don't agree this has anything to do ...
4 years, 10 months ago (2015-03-04 14:51:35 UTC) #10
Oleksandr
> If you want to insist that I split these up, you can, and I ...
4 years, 10 months ago (2015-03-04 21:02:59 UTC) #11
Oleksandr
http://codereview.adblockplus.org/5747779603267584/diff/5754903989321728/src/plugin/PluginDebug.cpp File src/plugin/PluginDebug.cpp (right): http://codereview.adblockplus.org/5747779603267584/diff/5754903989321728/src/plugin/PluginDebug.cpp#newcode158 src/plugin/PluginDebug.cpp:158: DEBUG_SELFTEST(L"********************************************************************************\n" + finalError + "\ n********************************************************************************") What's this for? ...
4 years, 10 months ago (2015-03-04 21:03:06 UTC) #12
Eric
New patch set fixes stray space. http://codereview.adblockplus.org/5747779603267584/diff/5754903989321728/src/plugin/PluginDebug.cpp File src/plugin/PluginDebug.cpp (right): http://codereview.adblockplus.org/5747779603267584/diff/5754903989321728/src/plugin/PluginDebug.cpp#newcode158 src/plugin/PluginDebug.cpp:158: DEBUG_SELFTEST(L"********************************************************************************\n" + finalError ...
4 years, 10 months ago (2015-03-05 00:47:11 UTC) #13
Eric
On 2015/03/04 21:02:59, Oleksandr wrote: > I'll stay on my point and still state that ...
4 years, 10 months ago (2015-03-05 01:07:53 UTC) #14
Oleksandr
4 years, 10 months ago (2015-03-06 03:22:24 UTC) #15
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5