You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2015/12/04 22:50:21 UTC
Review Request 40988: stout: Fixed comments for numify(),
cleaned up some test code.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40988/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Cong Wang.
Repository: mesos
Description
-------
stout: Fixed comments for numify(), cleaned up some test code.
Note that numify() handles negative numbers inconsistently, depending on whether they are specified in hex or decimal (see test case). Can you guys take a look at fixing?
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 322a89946befb0d7006124a08b415e2ef0a03e97
3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 522fc3470eb3496f7dfe1eeb814b171bcff21f38
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 7fa06a980b58040f68bf92d217c866f9e48a57d3
3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7c0309c41ee5ad18bed30aa31a8361f11bca23a1
Diff: https://reviews.apache.org/r/40988/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 40988: stout: Fixed comments for numify(),
cleaned up some test code.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40988/#review109033
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> stout: Fixed comments for numify(), cleaned up some test code.
>
> Note that numify() handles negative numbers inconsistently, depending on whether they are specified in hex or decimal (see test case). Can you guys take a look at fixing?
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 322a89946befb0d7006124a08b415e2ef0a03e97
> 3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 522fc3470eb3496f7dfe1eeb814b171bcff21f38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 7fa06a980b58040f68bf92d217c866f9e48a57d3
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7c0309c41ee5ad18bed30aa31a8361f11bca23a1
>
> Diff: https://reviews.apache.org/r/40988/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 40988: stout: Fixed comments for numify(),
cleaned up some test code.
Posted by Cong Wang <xi...@gmail.com>.
> On March 18, 2016, 11:13 p.m., Cong Wang wrote:
> > Can you separate the non-numify() change from the numify() one? It is best to do one thing per commit.
>
> Neil Conway wrote:
> This change was committed a few months ago.
I know it, I noticed this one because of 45011. And my comment is for Jie. I know that Mesos is still too young to care about bisect, but it is a best practice to check in one thing within one commit, this patch/commit does at least two things: one for numify(), one for the rest; it also hurts the readablity of this patch a little.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40988/#review124320
-----------------------------------------------------------
On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> stout: Fixed comments for numify(), cleaned up some test code.
>
> Note that numify() handles negative numbers inconsistently, depending on whether they are specified in hex or decimal (see test case). Can you guys take a look at fixing?
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 322a89946befb0d7006124a08b415e2ef0a03e97
> 3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 522fc3470eb3496f7dfe1eeb814b171bcff21f38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 7fa06a980b58040f68bf92d217c866f9e48a57d3
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7c0309c41ee5ad18bed30aa31a8361f11bca23a1
>
> Diff: https://reviews.apache.org/r/40988/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 40988: stout: Fixed comments for numify(),
cleaned up some test code.
Posted by Neil Conway <ne...@gmail.com>.
> On March 18, 2016, 11:13 p.m., Cong Wang wrote:
> > Can you separate the non-numify() change from the numify() one? It is best to do one thing per commit.
This change was committed a few months ago.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40988/#review124320
-----------------------------------------------------------
On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> stout: Fixed comments for numify(), cleaned up some test code.
>
> Note that numify() handles negative numbers inconsistently, depending on whether they are specified in hex or decimal (see test case). Can you guys take a look at fixing?
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 322a89946befb0d7006124a08b415e2ef0a03e97
> 3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 522fc3470eb3496f7dfe1eeb814b171bcff21f38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 7fa06a980b58040f68bf92d217c866f9e48a57d3
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7c0309c41ee5ad18bed30aa31a8361f11bca23a1
>
> Diff: https://reviews.apache.org/r/40988/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 40988: stout: Fixed comments for numify(),
cleaned up some test code.
Posted by Neil Conway <ne...@gmail.com>.
> On March 18, 2016, 11:13 p.m., Cong Wang wrote:
> > Can you separate the non-numify() change from the numify() one? It is best to do one thing per commit.
>
> Neil Conway wrote:
> This change was committed a few months ago.
>
> Cong Wang wrote:
> I know it, I noticed this one because of 45011. And my comment is for Jie. I know that Mesos is still too young to care about bisect, but it is a best practice to check in one thing within one commit, this patch/commit does at least two things: one for numify(), one for the rest; it also hurts the readablity of this patch a little.
Sure, I agree in general. I just mean that it is too late to separate the numify() from non-numify() changes at this point.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40988/#review124320
-----------------------------------------------------------
On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> stout: Fixed comments for numify(), cleaned up some test code.
>
> Note that numify() handles negative numbers inconsistently, depending on whether they are specified in hex or decimal (see test case). Can you guys take a look at fixing?
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 322a89946befb0d7006124a08b415e2ef0a03e97
> 3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 522fc3470eb3496f7dfe1eeb814b171bcff21f38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 7fa06a980b58040f68bf92d217c866f9e48a57d3
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7c0309c41ee5ad18bed30aa31a8361f11bca23a1
>
> Diff: https://reviews.apache.org/r/40988/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 40988: stout: Fixed comments for numify(),
cleaned up some test code.
Posted by Cong Wang <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40988/#review124320
-----------------------------------------------------------
Can you separate the non-numify() change from the numify() one? It is best to do one thing per commit.
- Cong Wang
On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> stout: Fixed comments for numify(), cleaned up some test code.
>
> Note that numify() handles negative numbers inconsistently, depending on whether they are specified in hex or decimal (see test case). Can you guys take a look at fixing?
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 322a89946befb0d7006124a08b415e2ef0a03e97
> 3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 522fc3470eb3496f7dfe1eeb814b171bcff21f38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 7fa06a980b58040f68bf92d217c866f9e48a57d3
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7c0309c41ee5ad18bed30aa31a8361f11bca23a1
>
> Diff: https://reviews.apache.org/r/40988/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>