Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6224768520945664: Issue #276 - conversion functions between std::wstring and ATL::CString (Closed)

Created:
Aug. 1, 2014, 2:33 p.m. by Eric
Modified:
Oct. 1, 2014, 7:43 a.m.
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+48 lines, -0 lines) Patch
M adblockplus.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M src/plugin/ATL_Deprecate.h View 2 chunks +31 lines, -0 lines 0 comments Download
A src/plugin/ATL_Deprecate.cpp View 1 chunk +16 lines, -0 lines 3 comments Download

Messages

Total messages: 8
Eric
Aug. 1, 2014, 2:36 p.m. (2014-08-01 14:36:32 UTC) #1
Oleksandr
LGTM
Aug. 5, 2014, 1:09 p.m. (2014-08-05 13:09:13 UTC) #2
sergei
LGTM
Aug. 5, 2014, 1:23 p.m. (2014-08-05 13:23:05 UTC) #3
Felix Dahlke
Sorry for being late to the party, but I think this needs some more work... ...
Sept. 30, 2014, 1:21 p.m. (2014-09-30 13:21:36 UTC) #4
Eric
http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp File src/plugin/ATL_Deprecate.cpp (right): http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp#newcode7 src/plugin/ATL_Deprecate.cpp:7: std::wstring to_wstring(const ATL::CString& s) On 2014/09/30 13:21:36, Felix H. ...
Sept. 30, 2014, 3:43 p.m. (2014-09-30 15:43:34 UTC) #5
Felix Dahlke
On 2014/09/30 15:43:34, Eric wrote: > http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp > File src/plugin/ATL_Deprecate.cpp (right): > > http://codereview.adblockplus.org/6224768520945664/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp#newcode7 > ...
Sept. 30, 2014, 4:17 p.m. (2014-09-30 16:17:37 UTC) #6
Eric
On 2014/09/30 16:17:37, Felix H. Dahlke wrote: > Sounds like a simple rename to me ...
Sept. 30, 2014, 4:50 p.m. (2014-09-30 16:50:31 UTC) #7
Felix Dahlke
Oct. 1, 2014, 7:43 a.m. (2014-10-01 07:43:45 UTC) #8
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.

Powered by Google App Engine
This is Rietveld