You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2015/02/03 00:40:19 UTC
Review Request 30531: Remove strings::format and unnecessary
constants from paths
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Bugs: MESOS-2314
https://issues.apache.org/jira/browse/MESOS-2314
Repository: mesos
Description
-------
Remove strings::format and unnecessary constants from paths
Diffs
-----
src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
src/tests/paths_tests.cpp 417aa51b99d54055ad7254b4f274fb59d0794ca6
Diff: https://reviews.apache.org/r/30531/diff/
Testing
-------
Thanks,
Dominic Hamon
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/#review70659
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30531]
All tests passed.
- Mesos ReviewBot
On Feb. 2, 2015, 11:40 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30531/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2015, 11:40 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-2314
> https://issues.apache.org/jira/browse/MESOS-2314
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove strings::format and unnecessary constants from paths
>
>
> Diffs
> -----
>
> src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
> src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
> src/tests/paths_tests.cpp 417aa51b99d54055ad7254b4f274fb59d0794ca6
>
> Diff: https://reviews.apache.org/r/30531/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/
-----------------------------------------------------------
(Updated Feb. 4, 2015, 9:40 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-2314
https://issues.apache.org/jira/browse/MESOS-2314
Repository: mesos
Description
-------
Remove strings::format and unnecessary constants from paths
Diffs
-----
src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
Diff: https://reviews.apache.org/r/30531/diff/
Testing (updated)
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/#review70984
-----------------------------------------------------------
Ship it!
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116466>
please use const reference here
const string&
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116468>
Ditto.
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116469>
Ditto.
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116470>
Ditto.
- Jie Yu
On Feb. 4, 2015, 5:22 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30531/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 5:22 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-2314
> https://issues.apache.org/jira/browse/MESOS-2314
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove strings::format and unnecessary constants from paths
>
>
> Diffs
> -----
>
> src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
> src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
>
> Diff: https://reviews.apache.org/r/30531/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/
-----------------------------------------------------------
(Updated Feb. 4, 2015, 9:22 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
just the code change, no test changes.
Bugs: MESOS-2314
https://issues.apache.org/jira/browse/MESOS-2314
Repository: mesos
Description
-------
Remove strings::format and unnecessary constants from paths
Diffs (updated)
-----
src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
Diff: https://reviews.apache.org/r/30531/diff/
Testing
-------
Thanks,
Dominic Hamon
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 4, 2015, 1:32 a.m., Jie Yu wrote:
> > src/slave/paths.cpp, line 453
> > <https://reviews.apache.org/r/30531/diff/1/?file=844643#file844643line453>
> >
> > const string& directory
>
> Dominic Hamon wrote:
> the function returns a string by value. there's no improvement here using a const ref (the copy will be elided). using a plan string is much clearer i think.
Does it save a call to the copy constructor (from the temp object to 'directory')? Or the compiler will optimize it?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/#review70885
-----------------------------------------------------------
On Feb. 4, 2015, 5:22 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30531/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 5:22 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-2314
> https://issues.apache.org/jira/browse/MESOS-2314
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove strings::format and unnecessary constants from paths
>
>
> Diffs
> -----
>
> src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
> src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
>
> Diff: https://reviews.apache.org/r/30531/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 4, 2015, 1:32 a.m., Jie Yu wrote:
> > src/slave/paths.cpp, line 453
> > <https://reviews.apache.org/r/30531/diff/1/?file=844643#file844643line453>
> >
> > const string& directory
>
> Dominic Hamon wrote:
> the function returns a string by value. there's no improvement here using a const ref (the copy will be elided). using a plan string is much clearer i think.
>
> Jie Yu wrote:
> Does it save a call to the copy constructor (from the temp object to 'directory')? Or the compiler will optimize it?
OK, I found this
http://en.wikipedia.org/wiki/Copy_elision
As long as the copy constructor of std::string does not have side effect, the copy will be elided. Look good to me. Feel free to commit.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/#review70885
-----------------------------------------------------------
On Feb. 4, 2015, 5:22 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30531/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 5:22 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-2314
> https://issues.apache.org/jira/browse/MESOS-2314
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove strings::format and unnecessary constants from paths
>
>
> Diffs
> -----
>
> src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
> src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
>
> Diff: https://reviews.apache.org/r/30531/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Feb. 3, 2015, 5:32 p.m., Jie Yu wrote:
> > src/slave/paths.cpp, line 453
> > <https://reviews.apache.org/r/30531/diff/1/?file=844643#file844643line453>
> >
> > const string& directory
the function returns a string by value. there's no improvement here using a const ref (the copy will be elided). using a plan string is much clearer i think.
> On Feb. 3, 2015, 5:32 p.m., Jie Yu wrote:
> > src/tests/paths_tests.cpp, lines 102-103
> > <https://reviews.apache.org/r/30531/diff/1/?file=844644#file844644line102>
> >
> > Two lines between top level functions. Here and everywhere else.
fixing these in https://reviews.apache.org/r/30624/
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/#review70885
-----------------------------------------------------------
On Feb. 4, 2015, 9:22 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30531/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 9:22 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-2314
> https://issues.apache.org/jira/browse/MESOS-2314
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove strings::format and unnecessary constants from paths
>
>
> Diffs
> -----
>
> src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
> src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
>
> Diff: https://reviews.apache.org/r/30531/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 30531: Remove strings::format and unnecessary
constants from paths
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30531/#review70885
-----------------------------------------------------------
Can we seperate the tests changes in this patch? It is a good practice not to change tests for refactors (if not necessary) so that we can capture potentiall regressions and have more confidence about the refactor?
The rest looks good to me! Thanks for the cleanup!
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116331>
const string& directory
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116332>
const string& last
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116333>
ditto.
src/slave/paths.cpp
<https://reviews.apache.org/r/30531/#comment116334>
ditto.
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116339>
Two lines between top level functions. Here and everywhere else.
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116335>
const string&
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116337>
`{` should be in next line. Here and everywhere else.
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116340>
const string&
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116346>
const string&
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116342>
The format here is a little jagged, how about
```
EXPECT_EQ(
path::join(
executorsRoot,
executorId.value(),
"runs",
containerId.value()),
paths::getExecutorRunPath(
rootDir,
slaveId,
frameworkId,
executorId,
containerId));
```
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116344>
Ditto here on the jaggedness.
src/tests/paths_tests.cpp
<https://reviews.apache.org/r/30531/#comment116345>
const string&
- Jie Yu
On Feb. 2, 2015, 11:40 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30531/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2015, 11:40 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-2314
> https://issues.apache.org/jira/browse/MESOS-2314
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove strings::format and unnecessary constants from paths
>
>
> Diffs
> -----
>
> src/slave/paths.hpp 4d6897f9eebcd6852f0223bae23a29729a42e750
> src/slave/paths.cpp fe951abac1254748dbd5bdd81b7e50da75afad6d
> src/tests/paths_tests.cpp 417aa51b99d54055ad7254b4f274fb59d0794ca6
>
> Diff: https://reviews.apache.org/r/30531/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dominic Hamon
>
>