CPP: libphonenumber 4.3

CPP: libphonenumber 4.3

Committed: http://code.google.com/p/libphonenumber/source/detail?r=403

[lararennie, 2011-11-28 16:38:17]

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
File cpp/src/phonenumbers/phonenumberutil.cc (right):

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
cpp/src/phonenumbers/phonenumberutil.cc:952: } else if ((region_code == "PE") &&
Can this fit on one line? It looks just a little odd....  if it is like this
internally, I probably was doing it like Brazil for cos initially there was some
fixed_line/mobile restrictions...

[philippe, 2011-11-28 17:07:20]

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
File cpp/src/phonenumbers/phonenumberutil.cc (right):

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
cpp/src/phonenumbers/phonenumberutil.cc:952: } else if ((region_code == "PE") &&
On 2011/11/28 16:38:17, lararennie wrote:
> Can this fit on one line? It looks just a little odd....  if it is like this
> internally, I probably was doing it like Brazil for cos initially there was
some
> fixed_line/mobile restrictions...

Internally it uses RegionCode therefore the test expression doesn't fit on a
single line. I kept the initial formatting to make the external code looks the
same as the internal one.
What do you want me to do?

[lararennie, 2011-11-28 17:11:41]

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
File cpp/src/phonenumbers/phonenumberutil.cc (right):

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
cpp/src/phonenumbers/phonenumberutil.cc:952: } else if ((region_code == "PE") &&
On 2011/11/28 17:07:21, philippe wrote:
> On 2011/11/28 16:38:17, lararennie wrote:
> > Can this fit on one line? It looks just a little odd....  if it is like this
> > internally, I probably was doing it like Brazil for cos initially there was
> some
> > fixed_line/mobile restrictions...
> 
> Internally it uses RegionCode therefore the test expression doesn't fit on a
> single line. I kept the initial formatting to make the external code looks the
> same as the internal one.
> What do you want me to do?

What about for CO? What do we do internally there? It would be good to have both
consistent...

[philippe, 2011-11-28 17:26:24]

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
File cpp/src/phonenumbers/phonenumberutil.cc (right):

http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
cpp/src/phonenumbers/phonenumberutil.cc:952: } else if ((region_code == "PE") &&
On 2011/11/28 17:11:42, lararennie wrote:
> On 2011/11/28 17:07:21, philippe wrote:
> > On 2011/11/28 16:38:17, lararennie wrote:
> > > Can this fit on one line? It looks just a little odd....  if it is like
this
> > > internally, I probably was doing it like Brazil for cos initially there
was
> > some
> > > fixed_line/mobile restrictions...
> > 
> > Internally it uses RegionCode therefore the test expression doesn't fit on a
> > single line. I kept the initial formatting to make the external code looks
the
> > same as the internal one.
> > What do you want me to do?
> 
> What about for CO? What do we do internally there? It would be good to have
both
> consistent...

CO is on a single line internally (80 cols).
I made PE and BR fit on a single line for consistency in the open-source code.
It's probably not worth keeping them on separate lines as the lines are
different from internally anyway (string vs RegionCode).

[lararennie, 2011-11-28 17:28:49]

On 2011/11/28 17:26:24, philippe wrote:
>
http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
> File cpp/src/phonenumbers/phonenumberutil.cc (right):
> 
>
http://codereview.appspot.com/5445043/diff/1/cpp/src/phonenumbers/phonenumber...
> cpp/src/phonenumbers/phonenumberutil.cc:952: } else if ((region_code == "PE")
&&
> On 2011/11/28 17:11:42, lararennie wrote:
> > On 2011/11/28 17:07:21, philippe wrote:
> > > On 2011/11/28 16:38:17, lararennie wrote:
> > > > Can this fit on one line? It looks just a little odd....  if it is like
> this
> > > > internally, I probably was doing it like Brazil for cos initially there
> was
> > > some
> > > > fixed_line/mobile restrictions...
> > > 
> > > Internally it uses RegionCode therefore the test expression doesn't fit on
a
> > > single line. I kept the initial formatting to make the external code looks
> the
> > > same as the internal one.
> > > What do you want me to do?
> > 
> > What about for CO? What do we do internally there? It would be good to have
> both
> > consistent...
> 
> CO is on a single line internally (80 cols).
> I made PE and BR fit on a single line for consistency in the open-source code.
> It's probably not worth keeping them on separate lines as the lines are
> different from internally anyway (string vs RegionCode).

OK, LGTM either way :)

[Shaopeng, 2011-11-29 08:41:44]

I just noticed that we forgot to port the recent 2 changes of the
AsYouTypeFormatter to C++. They are really small, so could you port it to C++
and add them to this CL, so that the opensouced version of AYTF stays the same?

Thanks,

Shaopeng

[philippe, 2011-11-29 10:33:22]

On 2011/11/29 08:41:44, Shaopeng wrote:
> I just noticed that we forgot to port the recent 2 changes of the
> AsYouTypeFormatter to C++. They are really small, so could you port it to C++
> and add them to this CL, so that the opensouced version of AYTF stays the
same?
> 
> Thanks,
> 
> Shaopeng

I have just added the changes related to the AYTF.

[Shaopeng, 2011-11-29 10:37:23]

LGTM