Issue #1234 - Rework strings in debug facility

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.

[Eric, 2015-02-24 15:23:30]

Some notes on out-of-scope changes that might be combined here, but are not:

-- I might have rewritten the innards of the legacy functions in PluginDebug.cpp
to eliminate CString from that file. I decided the present change set was
getting too large, so I just forwarded the new functions to the old ones, adding
type conversions. Changing this file

-- We have debugging definitions spread out over multiple files and two classes.
I've eliminated one of them in this change set, which takes us from five files
down to four four. Combining the rest of them is for the future; it would be too
much text modification to try to do them all here.

-- 'CPluginDebug' is a class containing only static functions and should really
be a namespace or possibly a singleton. We can change this later; it causes no
harm now. It would also be a premature change, since we have a log queue to
integrate still.

-- We should stop using the preprocessor for conditional compilation of
debug-only statements and replace it with a template mechanism. Turning off
compilation entirely is prone to bit rot, since argument expressions and type
matching are not checked. Using a template mechanism ensures that the arguments
are checked, even if the function body is empty and it's optimized out.

[sergei, 2015-02-25 13:53:31]

Great!

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
File src/plugin/Config.h (right):

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description)
CPluginClientBase::PostPluginError(err, id, subid, description);
Why is it changed from Log to Post?

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
src/plugin/PluginTabBase.cpp:326: message += std::to_string(
::GetCurrentProcessId() );
here and below additional spaces

[Eric, 2015-02-25 15:24:46]

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
File src/plugin/Config.h (right):

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description)
CPluginClientBase::PostPluginError(err, id, subid, description);
On 2015/02/25 13:53:31, sergei wrote:
> Why is it changed from Log to Post?

The change is both a clarification and an optimization.

The optimization: The implementation of 'LogPluginError' has seven arguments,
the last three with default values. The fifth argument, 'isAsync', defaults to
false and controls flow within the function. This macro, not specifying the
value, uses the default. With the default value, the only behavior of
'LogPluginError' is to call 'PostPluginError'.

The clarification: In other words, use of this macro always uses the log queue.

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
src/plugin/PluginTabBase.cpp:326: message += std::to_string(
::GetCurrentProcessId() );
On 2015/02/25 13:53:31, sergei wrote:
> here and below additional spaces

Done.

[sergei, 2015-02-25 15:51:23]

LGTM

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
File src/plugin/Config.h (right):

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description)
CPluginClientBase::PostPluginError(err, id, subid, description);
On 2015/02/25 15:24:46, Eric wrote:
> On 2015/02/25 13:53:31, sergei wrote:
> > Why is it changed from Log to Post?
> 
> The change is both a clarification and an optimization.
> 
> The optimization: The implementation of 'LogPluginError' has seven arguments,
> the last three with default values. The fifth argument, 'isAsync', defaults to
> false and controls flow within the function. This macro, not specifying the
> value, uses the default. With the default value, the only behavior of
> 'LogPluginError' is to call 'PostPluginError'.
> 
> The clarification: In other words, use of this macro always uses the log
queue.

Clear.
However, I would say that impl of `CPluginClientBase::LogPluginError` is
incorrect regarding `isAsync` because it should queue it if `isAsync` is true.
Anyway, it's not related to this issue, so it's OK.

[Eric, 2015-02-25 16:13:59]

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
File src/plugin/Config.h (right):

http://codereview.adblockplus.org/5747779603267584/diff/5136918324969472/src/...
src/plugin/Config.h:96: #define DEBUG_ERROR_LOG(err, id, subid, description)
CPluginClientBase::PostPluginError(err, id, subid, description);
On 2015/02/25 15:51:23, sergei wrote:
> However, I would say that impl of `CPluginClientBase::LogPluginError` is
> incorrect regarding `isAsync` because it should queue it if `isAsync` is true.
> Anyway, it's not related to this issue, so it's OK.

Yes, I agree. The sense of 'isAsync' is backwards. 

In another pending review, these functions are part of the rump of
'CPluginClientBase' which is getting renamed to 'LogQueue'. That queue object
needs a rewrite, and we'll fix this naming issue then, likely by eliminating the
argument entirely with a new class structure with separate functions for
synchronous and asynchronous reporting.

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

This patch-set should have really been split into two, IMO. One for actual
CString removal and another one for re-factoring Debug macros, which is
unrelated. As it is now this infringes on issue
https://issues.adblockplus.org/ticket/1357. 

Also, I don't think we should be leaving CStrings within the implementation of
debug functions here. It only adds unnecessary twists to code for no particular
reason.

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

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:54: void DebugLegacy(const CString& text, DWORD
dwProcessId, DWORD dwThreadId)
Isn't this changeset about converting CStrings to std strings in Debug
functions? Why do we need this legacy function?

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:88: ::GetSystemTime(&st);
Probably easier to replace this with std::localtime(std::time(nullptr)) and then
use std::strftime

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:104: CStringA line = text.Tokenize(L"\n\r", pos);
Looks like just a place for wcstok_s again

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:147: void DebugErrorCodeLegacy(DWORD errorCode, const
CString& error, DWORD dwProcessId, DWORD dwThreadId)
Same here. Why do we need legacy functions after this changeset?

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

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginTabBase.cpp:132: 0 ==
urlLegacy.CompareNoCase(CString(FirstRunPageFileUrl().c_str()))))
I really don't think there are many other options here besides using _wcsnicmp.
Why not just use that? This is the only place where urlLegacy is used.

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

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

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:54: void DebugLegacy(const CString& text, DWORD
dwProcessId, DWORD dwThreadId)
On 2015/02/27 07:50:15, Oleksandr wrote:
> Isn't this changeset about converting CStrings to std strings in Debug
> functions? Why do we need this legacy function?

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.

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:88: ::GetSystemTime(&st);
On 2015/02/27 07:50:15, Oleksandr wrote:
> Probably easier to replace this with std::localtime(std::time(nullptr)) and
then
> use std::strftime

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

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:104: CStringA line = text.Tokenize(L"\n\r", pos);
On 2015/02/27 07:50:15, Oleksandr wrote:
> Looks like just a place for wcstok_s again

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.

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginDebug.cpp:147: void DebugErrorCodeLegacy(DWORD errorCode, const
CString& error, DWORD dwProcessId, DWORD dwThreadId)
On 2015/02/27 07:50:15, Oleksandr wrote:
> Same here. Why do we need legacy functions after this changeset?

See above. Same reason.

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

http://codereview.adblockplus.org/5747779603267584/diff/5733935958982656/src/...
src/plugin/PluginTabBase.cpp:132: 0 ==
urlLegacy.CompareNoCase(CString(FirstRunPageFileUrl().c_str()))))
On 2015/02/27 07:50:15, Oleksandr wrote:
> I really don't think there are many other options here besides using
_wcsnicmp.
> Why not just use that? This is the only place where urlLegacy is used.

I changed only the code relevant to the DEBUG_GENERAL statement. I wasn't even
looking at the remaining uses of that variable.

We could change it here if you'd like, even if it's not strictly related.

[Eric, 2015-02-27 14:40:31]

On 2015/02/27 07:50:15, Oleksandr wrote:
> This patch-set should have really been split into two, IMO. One for actual
> CString removal and another one for re-factoring Debug macros, which is
> unrelated.

Well, it is related, since I was having trouble auditing the code before I
simplified those macros. They're certainly not perfect still, but at least
they're easier to follow.

> As it is now this infringes on issue
> https://issues.adblockplus.org/ticket/1357. 

I hadn't read that issue in detail. I saw and agreed with the title, that those
DEBUG_* macros should go, but I don't agree with the suggested way of doing it,
which simply to collapse the macros definitions completely as if the symbol
'_DEBUG' were defined. I dislike using the preprocessor except where
unavoidable, but these macros at least have the merit that they suppress
generation of debugging code in Release builds.

We don't need to use the preprocessor to get conditional compilation; we can use
the template mechanism. Without getting into too much detail here, the key is to
use a template function with an empty body for the default and a template
specialization for debug configurations that calls the appropriate function.

> Also, I don't think we should be leaving CStrings within the implementation of
> debug functions here. It only adds unnecessary twists to code for no
particular
> reason.

As I mentioned in my initial message (right after creating the review) and also
in the comments, I split the work into two parts. I'm not making any kind of
claim that keeping CString within the implementation is any kind of long term
plan.

[Oleksandr, 2015-03-03 20:49:33]

> Well, it is related, since I was having trouble auditing the code before I
> simplified those macros. They're certainly not perfect still, but at least
> they're easier to follow.

I still don't agree this has anything to do to the reworking of strings. Some
DEBUG_ macros changes would be expected (to accommodate a new type), but not
re-factoring like this. 

> > As it is now this infringes on issue
> > https://issues.adblockplus.org/ticket/1357. 
> 
> I hadn't read that issue in detail. I saw and agreed with the title, that
those
> DEBUG_* macros should go, but I don't agree with the suggested way of doing
it,
> which simply to collapse the macros definitions completely as if the symbol
> '_DEBUG' were defined. 

This looks like a comment for the issue #1357, not here. I think I can agree
that just collapsing the _DEBUG define isn't great though.

> As I mentioned in my initial message (right after creating the review) and
also
> in the comments, I split the work into two parts. I'm not making any kind of
> claim that keeping CString within the implementation is any kind of long term
> plan.

I would agree with this if we hadn't had quite a few instances of 'legacy'
variables in the code already without this change. To anyone looking into the
code those are there for completely no reason and it is not clear when they will
be gone. In this particular case I have tried to show that just removing all
CStrings isn't that big of a task, and we can just do it in this patchset.
Removing all CStrings certainly looks more related to this change than reworking
the DEBUG_ macros, IMO. As it is now, this changeset doesn't really remove
CStrings nor does it really refactor the DEBUG_ marcos.

[Eric, 2015-03-04 14:51:35]

On 2015/03/03 20:49:33, Oleksandr wrote:
> I still don't agree this has anything to do to the reworking of strings. Some
> DEBUG_ macros changes would be expected (to accommodate a new type), but not
> re-factoring like this. 

What do you want here? The alternative is to split up this change set into two
parts, where changing the DEBUG macros gets a "Noissue" annotation because it's
not actually doing anything to resolve #1357 at all. All it's doing is
refactoring their current implementation; it's not fixing the issue, not even
partially.

Now I see, reading #1357, that there's an open pull request on it. Looking at
the code in that request, it looks like a regression to me. It would compile all
the debug calls into Release code. Irrespective of the present review, I'd
recommend not to accept that request. And with respect to this review, it
infringes on #1357 only to the extent that it modifies code related to a pull
request that I think it's a bad idea to accept.

If you want to insist that I split these up, you can, and I will be really
cranky while doing it, because it seems like utterly wasted effort to do so. I
don't see any comments from you on the code itself. Splitting things up would
lead to the same changes in code, just in two commits rather than one for what
seem like slim and marginal administrative benefits.

> I would agree with this if we hadn't had quite a few instances of 'legacy'
> variables in the code already without this change. To anyone looking into the
> code those are there for completely no reason and it is not clear when they
will
> be gone.

If you'd like them to be gone sooner, increase the speed of reviewing.

I already did the work once (last summer), in one big change that everyone
thought was too large, but had no legacy CString variables. Splitting up the
work is necessarily slower not just because there are more reviews and more
delays. It requires serializing the changes. Because of cascading dependencies,
it's far too much work to have more than one open CString review at a time. And
even simple ones seem to take a week or two to complete.

> In this particular case I have tried to show that just removing all
> CStrings isn't that big of a task, and we can just do it in this patchset.

It's certainly possible, and it would make this change set too large. I think
this is the first time I've gotten a complaint that a change set was too small.
(And at the same time complaining that progress is too slow, to boot.)

You haven't shown at all that removing them from the implementation isn't that
big, and you seem to have ignored my responses about why it's larger than you
think. So let me reiterate. PluginDebug.cpp uses CString member functions that
do not have 'std::basic_string' equivalents. Replacing them would require not
only adding them but adding unit tests for them. That's extra work and extra
files and extra testing and I deemed it out of scope for the present change set.

> As it is now, this changeset doesn't really remove
> CStrings nor does it really refactor the DEBUG_ marcos.

Please re-read the first paragraph of the description. It removes CString from
the signatures of the debug functions, and thus also all the places they're
called. If that doesn't "really remove CStrings", then I don't know what that
phrase means. This is just like a number of other times where I've eliminated
CString first from signatures of public functions.

Refactoring means changing an implementation without changing functionality.
That is exactly what I've done with these macros, removing part of their
definitions from PluginDebugMacros.h and consolidating the definitions in
Config.h. What I have not done is rewritten them nor replaced them, nor did I
intend to.

[Oleksandr, 2015-03-04 21:02:59]

> If you want to insist that I split these up, you can, and I will be really
> cranky while doing it, because it seems like utterly wasted effort to do so. I
> don't see any comments from you on the code itself. Splitting things up would
> lead to the same changes in code, just in two commits rather than one for what
> seem like slim and marginal administrative benefits.

Ok, let's keep them in one change set. I merely wanted to point out that
logically this is 2 change sets. One of which is described in a different issue,
and discussion on it should be there. I'll add a comment there.

> If you'd like them to be gone sooner, increase the speed of reviewing.
> 

Fair enough. However I would assume change set that are removing 'legacy'
variables would come before adding more legacy code.

> I think this is the first time I've gotten a complaint that a change set was
too small.

The complaint is that the change set is not focused on an issue, not that it is
too small.

> You haven't shown at all that removing them from the implementation isn't that
> big, and you seem to have ignored my responses about why it's larger than you
> think. So let me reiterate. PluginDebug.cpp uses CString member functions that
> do not have 'std::basic_string' equivalents. Replacing them would require not
> only adding them but adding unit tests for them. That's extra work and extra
> files and extra testing and I deemed it out of scope for the present change
set.

I'll stay on my point and still state that the changes I suggest would not be
too much. You only say that "we don't have drop-in replacement functions for
some of the
CString member functions", without really specifying which functions are those.

I don't see any point in having another lengthy discussion here though, so if
you insist we can get even more legacy variables in and remove them later.

Other than the conceptual stuff, and a small bug in comments the code looks good
to me. I will run this by Felix and update you here.

[Oleksandr, 2015-03-04 21:03:06]

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

http://codereview.adblockplus.org/5747779603267584/diff/5754903989321728/src/...
src/plugin/PluginDebug.cpp:158:
DEBUG_SELFTEST(L"********************************************************************************\n"
+ finalError + "\
n********************************************************************************")
What's this for? No space between '\' and 'n'

[Eric, 2015-03-05 00:47:11]

New patch set fixes stray space.

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

http://codereview.adblockplus.org/5747779603267584/diff/5754903989321728/src/...
src/plugin/PluginDebug.cpp:158:
DEBUG_SELFTEST(L"********************************************************************************\n"
+ finalError + "\
n********************************************************************************")
On 2015/03/04 21:03:06, Oleksandr wrote:
> What's this for? No space between '\' and 'n'

It's a typo. Sometimes stray characters end up in the editor when I've got the
wrong window selected.

Also, DEBUG_SELFTEST is currently defined to an empty macro body. I thought
about just removing it, but decided to defer that decision for later.

Fixed.

[Eric, 2015-03-05 01:07:53]

On 2015/03/04 21:02:59, Oleksandr wrote:
> I'll stay on my point and still state that the changes I suggest would not be
> too much. You only say that "we don't have drop-in replacement functions for
> some of the
> CString member functions", without really specifying which functions are
those.

Left, Right, Format, Tokenize. Nothing major, but that's a lot of detail work
that I'd rather not combine. 

The debug facility isn't very amenable to unit testing since its I/O isn't
virtualized; the files are opened in-line rather than provided externally. In
addition, the debug files are being opened with each call and there's duplicated
code to format the entries. There's a lock in place that serializes threads in
within a process but has race conditions between multiple IE processes.

I'm not feeling the need to rewrite all of this. I just felt it would be easier
to work on this file separately, and that meant first isolating the CString
dependencies. The next change set on this topic should only be changing this one
file.

> I don't see any point in having another lengthy discussion here though

Yes, let's not.

[Oleksandr, 2015-03-06 03:22:24]

LGTM