Issue #276 - conversion functions between std::wstring and ATL::CString

Issue #276 - conversion functions between std::wstring and ATL::CString

During an interim period while removing CString, it's beneficial to use explicit
conversion functions. Although more verbose, they clearly identify such
conversions instead of relying on implicit and idiomatic means. Explicit
functions also enable removal of intermediate code, since some of the other
conversions remain valid but inefficient when variable types are changed.

Supersedes in part http://codereview.adblockplus.org/5070706781978624/

Separated out to alleviate potential friction with commit dependencies.

[Oleksandr, 2014-08-05 13:09:13]

LGTM

[sergei, 2014-08-05 13:23:05]

LGTM

[Felix Dahlke, 2014-09-30 13:21:36]

Sorry for being late to the party, but I think this needs some more work...

http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/...
File src/plugin/ATL_Deprecate.cpp (right):

http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:7: std::wstring to_wstring(const ATL::CString& s)
Am I missing something, or why do we use underscore naming here? Should be
ToWstring here and ToCString below.

http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:9: std::wstring result(static_cast<const wchar_t
*>(s));
No space between wchar_t and * please.

[Eric, 2014-09-30 15:43:34]

http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/...
File src/plugin/ATL_Deprecate.cpp (right):

http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:7: std::wstring to_wstring(const ATL::CString& s)
On 2014/09/30 13:21:36, Felix H. Dahlke wrote:
> Am I missing something, or why do we use underscore naming here? Should be
> ToWstring here and ToCString below.

This code was originally written months ago, before we addressed the issues
around differences in style.

These functions are scheduled to disappear entirely once we've converted all the
remaining CString occurrences to std::wstring. When that happens, both of these
function definitions and their corresponding declarations will go away entirely.

While we could change these names at this point, it would be a bunch of work for
not much benefit. We'd need to change the names for all declaration, definition,
and usages, get that committed. That would delay the work on the conversion by
putting such a revision onto the critical path of resolving the issue.

Given the explicit transience of these functions, I'd recommend living with the
legacy nature of these names.

[Felix Dahlke, 2014-09-30 16:17:37]

On 2014/09/30 15:43:34, Eric wrote:
>
http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/...
> File src/plugin/ATL_Deprecate.cpp (right):
> 
>
http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/...
> src/plugin/ATL_Deprecate.cpp:7: std::wstring to_wstring(const ATL::CString& s)
> On 2014/09/30 13:21:36, Felix H. Dahlke wrote:
> > Am I missing something, or why do we use underscore naming here? Should be
> > ToWstring here and ToCString below.
> 
> This code was originally written months ago, before we addressed the issues
> around differences in style.
> 
> These functions are scheduled to disappear entirely once we've converted all
the
> remaining CString occurrences to std::wstring. When that happens, both of
these
> function definitions and their corresponding declarations will go away
entirely.
> 
> While we could change these names at this point, it would be a bunch of work
for
> not much benefit. We'd need to change the names for all declaration,
definition,
> and usages, get that committed. That would delay the work on the conversion by
> putting such a revision onto the critical path of resolving the issue.
> 
> Given the explicit transience of these functions, I'd recommend living with
the
> legacy nature of these names.

We really shouldn't commit new code that violates the coding style - and we
generally never do... We've been using the Mozilla C++ Coding Style for two
years now...

Sounds like a simple rename to me - currently there are zero occurrences of
to_wstring and two for to_CString. I suppose the bulk of this is in the patches
that didn't land yet.

I assumed we need to keep those around for a while, until we're not using
CString anywhere anymore. Doesn't sound like something we'll achieve in the near
future. If we can get rid of those this week, no point renaming them. If we
cannot, please do that.

[Eric, 2014-09-30 16:50:31]

On 2014/09/30 16:17:37, Felix H. Dahlke wrote:
> Sounds like a simple rename to me - currently there are zero occurrences of
> to_wstring and two for to_CString. I suppose the bulk of this is in the
patches
> that didn't land yet.

Yes, I'm just about to commit one, one that you approved just a few hours ago.
Two others are already mostly-reviewed.

> I assumed we need to keep those around for a while, until we're not using
> CString anywhere anymore. Doesn't sound like something we'll achieve in the
near
> future. If we can get rid of those this week, no point renaming them. If we
> cannot, please do that.

We could get rid of CString within a week if our review process were more
functional. There's no work here that I haven't done already. Originally I did
all the string conversions in one giant lump (see
http://codereview.adblockplus.org/5750789393874944/) that also rationalized (at
least to my sensibilities) all of the other string uses. The present series of
string conversions consists of incremental versions of work already done. If it
weren't for the need to review incremental changes we wouldn't even need these
conversion functions. 

And I've had difficulty getting reviews for this series of much simpler changes.
For example, http://codereview.adblockplus.org/5109791353470976/ took two weeks
to get its first comments for a change set that's only 13 lines.

What I am quite concerned with is adding in yet another change in the middle of
this process that's going to slow down things even further.

[Felix Dahlke, 2014-10-01 07:43:45]

On 2014/09/30 16:50:31, Eric wrote:
> On 2014/09/30 16:17:37, Felix H. Dahlke wrote:
> > Sounds like a simple rename to me - currently there are zero occurrences of
> > to_wstring and two for to_CString. I suppose the bulk of this is in the
> patches
> > that didn't land yet.
> 
> Yes, I'm just about to commit one, one that you approved just a few hours ago.
> Two others are already mostly-reviewed.
> 
> > I assumed we need to keep those around for a while, until we're not using
> > CString anywhere anymore. Doesn't sound like something we'll achieve in the
> near
> > future. If we can get rid of those this week, no point renaming them. If we
> > cannot, please do that.
> 
> We could get rid of CString within a week if our review process were more
> functional. There's no work here that I haven't done already. Originally I did
> all the string conversions in one giant lump (see
> http://codereview.adblockplus.org/5750789393874944/) that also rationalized
(at
> least to my sensibilities) all of the other string uses. The present series of
> string conversions consists of incremental versions of work already done. If
it
> weren't for the need to review incremental changes we wouldn't even need these
> conversion functions. 
> 
> And I've had difficulty getting reviews for this series of much simpler
changes.
> For example, http://codereview.adblockplus.org/5109791353470976/ took two
weeks
> to get its first comments for a change set that's only 13 lines.
> 
> What I am quite concerned with is adding in yet another change in the middle
of
> this process that's going to slow down things even further.

Yes, we've not managed to review refactorings with priority lately. But now that
the release is out and most of the important stuff is taken care of, all this
should land relatively quickly.

By "getting rid of those this week" I meant we'll have a review for it by then.
Since you said all the code is already under review, let's keep the names here,
not worth rebasing all the patches over.