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

Issue 5450068: CPP: Refactoring PhoneNumberUtil so that global scoped_ptr variables are now (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 4 months ago by philippe
Modified:
12 years, 4 months ago
Reviewers:
lararennie
CC:
Shaopeng, georgey1
Base URL:
https://libphonenumber.googlecode.com/svn/trunk
Visibility:
Public.

Description

CPP: Refactoring PhoneNumberUtil so that global scoped_ptr variables are now hidden in an internal class. Patch contributed by Lara Rennie. Committed: http://code.google.com/p/libphonenumber/source/detail?r=407

Patch Set 1 #

Total comments: 2

Patch Set 2 : Move logger initialization to the initializer list #

Unified diffs Side-by-side diffs Delta from patch set Stats (+371 lines, -336 lines) Patch
M cpp/src/phonenumbers/phonenumberutil.h View 5 chunks +8 lines, -6 lines 0 comments Download
M cpp/src/phonenumbers/phonenumberutil.cc View 1 43 chunks +363 lines, -330 lines 0 comments Download

Messages

Total messages: 4
philippe
12 years, 4 months ago (2011-12-02 12:08:59 UTC) #1
lararennie
http://codereview.appspot.com/5450068/diff/1/cpp/src/phonenumbers/phonenumberutil.cc File cpp/src/phonenumbers/phonenumberutil.cc (right): http://codereview.appspot.com/5450068/diff/1/cpp/src/phonenumbers/phonenumberutil.cc#newcode641 cpp/src/phonenumbers/phonenumberutil.cc:641: logger_.reset(new StdoutLogger()); Should do logger_ in the init list
12 years, 4 months ago (2011-12-02 14:12:00 UTC) #2
philippe
http://codereview.appspot.com/5450068/diff/1/cpp/src/phonenumbers/phonenumberutil.cc File cpp/src/phonenumbers/phonenumberutil.cc (right): http://codereview.appspot.com/5450068/diff/1/cpp/src/phonenumbers/phonenumberutil.cc#newcode641 cpp/src/phonenumbers/phonenumberutil.cc:641: logger_.reset(new StdoutLogger()); On 2011/12/02 14:12:00, lararennie wrote: > Should ...
12 years, 4 months ago (2011-12-02 14:32:02 UTC) #3
lararennie
12 years, 4 months ago (2011-12-02 14:42:56 UTC) #4
LGTM
Sign in to reply to this message.

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