> On Feb. 27, 2015, 5:20 p.m., Dominic Hamon wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 305 > > <https://reviews.apache.org/r/31471/diff/1-3/?file=877503#file877503line305> > > > > please don't overload create, especially when it might be possible to > > introduce a bug that implicitly converts an IP to a prefix (through uint32 > > conversion). > > > > i prefer the explicit naming. jie, i know you don't but maybe you can > > see the danger here?
Here are my thoughts: 1) The function names are unncessarily verbose. From the argument types, it's obvious that create(IP, IP) is the netmask version and create(IP, int) is the prefix version. 2) We aleady used 'explicit' for the IP(uint32_t) constructor. Even if not, the compiler will not perform implicit conversion if there is an exact match. So we have two safetynets here. I don't think we'll ever going to introduce IP->uint32 conversion. In summary, I still prefer less verbose naming. Let me know if you insist and we can sync up:) - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31471/#review74511 ----------------------------------------------------------- On Feb. 27, 2015, 9:22 a.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31471/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2015, 9:22 a.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, Jie Yu, Joris Van > Remoortere, and Niklas Nielsen. > > > Repository: mesos > > > Description > ------- > > see summary > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > 3293086a009a8f7cf7bd343eb7d3e85623636550 > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp > 9635bbc6f7dae1d75a780069fcc60fb706221053 > 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp > fb98317a68986cb1228c584a8cd83b07737895a8 > > Diff: https://reviews.apache.org/r/31471/diff/ > > > Testing > ------- > > > Thanks, > > Evelina Dumitrescu > >
