You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2015/06/02 16:43:20 UTC
Review Request 34943: Added validation to flags.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/
-----------------------------------------------------------
Review request for mesos.
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 61a405f225d14acbc38a80d35570426cb05a3d0a
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp a6e8ba943d97ae908122a444332155ebc6c7bb93
Diff: https://reviews.apache.org/r/34943/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 8, 2015, 2:37 a.m., Jojy Varghese wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 517
> > <https://reviews.apache.org/r/34943/diff/2/?file=979975#file979975line517>
> >
> > style question: Why not use static array declaration :
> > const char *args[] = {
> > "/path/to/program",
> > "blah"
> > };
> >
> > and also make the function argument const char**?
For consistency, i.e., because the rest of this file does it this way. But I would love it if you did a sweep of this file and claned them all up to do it the way you proposed in a follow up review!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86981
-----------------------------------------------------------
On June 11, 2015, 1:52 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 1:52 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 134453e91d39fd086baa9396ab42a002fed8b73e
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 59cde89b5e035807a510b331b85a8cd48da36ae3
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 4485e41feb8320d2ec7251b8b894ce437f61addd
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp cfe6d742560f50079fb1ed7346526d432615613c
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
> 3rdparty/libprocess/include/Makefile.am d01880c66ca544d62d3ada1ea79c8045884858da
> 3rdparty/libprocess/include/process/firewall.hpp 16ed852d07344e53c6f9d30d4cd7d99e07c6606d
> 3rdparty/libprocess/include/process/process.hpp e70dd388601bd26fcc82f66e0cb95924da8a8c2f
> 3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198
> 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058
> Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15
> docs/configuration.md aaf65bfa2848318c8d07c772ba2c521aa72c2677
> docs/engineering-principles-and-practices.md 6babb929ead758ee5187d5fc5760d084876c3298
> docs/home.md ee3d3e9a417d75af776dad3ea9627d3c6d780dca
> docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e
> docs/powered-by-mesos.md 0ee763ffe32e6667e4ba708dd275d9f1b5116815
> include/mesos/containerizer/containerizer.proto f16ccc89f83da28c413ccfa0687a06b7515a605c
> include/mesos/master/allocator.hpp 92de1af414321281b00eaa6f129e5e3e2c849448
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> src/Makefile.am 10b190288425d735b116ed2dd3f040e871c5cb18
> src/cli/mesos-ps ae48c8cde31e90024381ed3e6e22ec3bc96560b6
> src/examples/no_executor_framework.cpp 6c5c7d3121e3a2ea78f66dffdeaecb72cca6293c
> src/files/files.cpp 3cdd38a3c7122bd5e13c8928279d85ab1373a63e
> src/linux/routing/handle.hpp 052c7cc1a967797d245a275d08cc774f627398a5
> src/linux/routing/queueing/fq_codel.hpp 18c53b416718c27045986d939bb85f9fec731ca5
> src/linux/routing/queueing/fq_codel.cpp 446863391d1c6dd2f91f890596927a20ee59a35f
> src/main.dox d5b29cf96b18ba84ac39f1747c4a174a5bfbc2e1
> src/master/allocator/mesos/allocator.hpp 6cfa04650d91a80211cfbc0809236f9438926c78
> src/master/allocator/mesos/hierarchical.hpp c220ea673320d4d06403afa76f0eb9f3b7a4e5eb
> src/master/flags.hpp 55ed3a9ef84a39841305cca2ba6c5df45c1990e1
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp e91e8815471b49d3c649470197d6da3f06de62fb
> src/master/metrics.hpp 3d389e68f22e3d1e00bde0db4e25f897c79a2316
> src/master/metrics.cpp d2489c8897fb2f2f21f021ac5e7a2ada7997ea06
> src/messages/flags.hpp 41be419ba5593a600aa0c6c411210fa4faa829a8
> src/messages/flags.proto 5400c92297f252734789982d21d7117ef4c57a57
> src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
> src/slave/flags.hpp 6c24e56d15881b0e3aeec3d4824842cf57121fc6
> src/slave/flags.cpp 99142fb1cf9d1978df86fdbf612e656b8fe852a6
> src/slave/main.cpp c379243e01919a5ab30bb9dea1b738665ba4e746
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/master_contender_detector_tests.cpp af6f15a8a85f2a292f5a64286aaa986f7dc29eff
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/no_executor_framework_test.sh b5bb111ceb99d4dc836537516de57c1ba0582371
> src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242
> src/tests/routing_tests.cpp 4be9967d38177d9f7def5c0da98d4c4266e7f0b5
> src/tests/script.cpp bcc1fab912410237dfe903d7a36cad9323d625a0
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86981
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
<https://reviews.apache.org/r/34943/#comment139192>
style question: Why not use static array declaration :
const char *args[] = {
"/path/to/program",
"blah"
};
and also make the function argument const char**?
- Jojy Varghese
On June 5, 2015, 2:48 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:48 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 11, 2015, 6:45 p.m., Ben Mahler wrote:
> > This patch is mixing changes across libprocess, stout, and mesos.. :)
It just hadn't been rebased. Fixed now.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review87602
-----------------------------------------------------------
On June 14, 2015, 1:55 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 14, 2015, 1:55 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review87602
-----------------------------------------------------------
This patch is mixing changes across libprocess, stout, and mesos.. :)
- Ben Mahler
On June 11, 2015, 1:52 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 1:52 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 134453e91d39fd086baa9396ab42a002fed8b73e
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 59cde89b5e035807a510b331b85a8cd48da36ae3
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 4485e41feb8320d2ec7251b8b894ce437f61addd
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp cfe6d742560f50079fb1ed7346526d432615613c
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
> 3rdparty/libprocess/include/Makefile.am d01880c66ca544d62d3ada1ea79c8045884858da
> 3rdparty/libprocess/include/process/firewall.hpp 16ed852d07344e53c6f9d30d4cd7d99e07c6606d
> 3rdparty/libprocess/include/process/process.hpp e70dd388601bd26fcc82f66e0cb95924da8a8c2f
> 3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198
> 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058
> Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15
> docs/configuration.md aaf65bfa2848318c8d07c772ba2c521aa72c2677
> docs/engineering-principles-and-practices.md 6babb929ead758ee5187d5fc5760d084876c3298
> docs/home.md ee3d3e9a417d75af776dad3ea9627d3c6d780dca
> docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e
> docs/powered-by-mesos.md 0ee763ffe32e6667e4ba708dd275d9f1b5116815
> include/mesos/containerizer/containerizer.proto f16ccc89f83da28c413ccfa0687a06b7515a605c
> include/mesos/master/allocator.hpp 92de1af414321281b00eaa6f129e5e3e2c849448
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> src/Makefile.am 10b190288425d735b116ed2dd3f040e871c5cb18
> src/cli/mesos-ps ae48c8cde31e90024381ed3e6e22ec3bc96560b6
> src/examples/no_executor_framework.cpp 6c5c7d3121e3a2ea78f66dffdeaecb72cca6293c
> src/files/files.cpp 3cdd38a3c7122bd5e13c8928279d85ab1373a63e
> src/linux/routing/handle.hpp 052c7cc1a967797d245a275d08cc774f627398a5
> src/linux/routing/queueing/fq_codel.hpp 18c53b416718c27045986d939bb85f9fec731ca5
> src/linux/routing/queueing/fq_codel.cpp 446863391d1c6dd2f91f890596927a20ee59a35f
> src/main.dox d5b29cf96b18ba84ac39f1747c4a174a5bfbc2e1
> src/master/allocator/mesos/allocator.hpp 6cfa04650d91a80211cfbc0809236f9438926c78
> src/master/allocator/mesos/hierarchical.hpp c220ea673320d4d06403afa76f0eb9f3b7a4e5eb
> src/master/flags.hpp 55ed3a9ef84a39841305cca2ba6c5df45c1990e1
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp e91e8815471b49d3c649470197d6da3f06de62fb
> src/master/metrics.hpp 3d389e68f22e3d1e00bde0db4e25f897c79a2316
> src/master/metrics.cpp d2489c8897fb2f2f21f021ac5e7a2ada7997ea06
> src/messages/flags.hpp 41be419ba5593a600aa0c6c411210fa4faa829a8
> src/messages/flags.proto 5400c92297f252734789982d21d7117ef4c57a57
> src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
> src/slave/flags.hpp 6c24e56d15881b0e3aeec3d4824842cf57121fc6
> src/slave/flags.cpp 99142fb1cf9d1978df86fdbf612e656b8fe852a6
> src/slave/main.cpp c379243e01919a5ab30bb9dea1b738665ba4e746
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/master_contender_detector_tests.cpp af6f15a8a85f2a292f5a64286aaa986f7dc29eff
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/no_executor_framework_test.sh b5bb111ceb99d4dc836537516de57c1ba0582371
> src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242
> src/tests/routing_tests.cpp 4be9967d38177d9f7def5c0da98d4c4266e7f0b5
> src/tests/script.cpp bcc1fab912410237dfe903d7a36cad9323d625a0
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 15, 2015, 10:17 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 146
> > <https://reviews.apache.org/r/34943/diff/4/?file=984516#file984516line146>
> >
> > Could we use some more concrete typing like this here?
> >
> > std::function<Option<Error>(Option<T>)>&& validate
> >
> > Similar types could then also be used in all other places where "F&&" appears below.
I can't use a well defined type here because the compiler can't deduce the 'T' inside Option<T> all by itself when using lambdas, instead the compiler needs the caller to explicitly specify the parameter type, which unfortunately makes all the call sites extremely more verbose (e.g., 'add<std::string>(&name, "name", "Description.", "default");').
I'm going to do another pass here and add documentation during which I'll clearly capture the expected type of F.
> On June 15, 2015, 10:17 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 293
> > <https://reviews.apache.org/r/34943/diff/4/?file=984516#file984516line293>
> >
> > What if T1 is Path? Then fetch() only parses. BTW: what does this mean then? This does not seem to be explained in fetch.hpp or anywhere else.
I updated the comment here. And yes, we need more documentation around 'fetch' and 'parse' too. On it's way!
> On June 15, 2015, 10:17 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 286
> > <https://reviews.apache.org/r/34943/diff/4/?file=984516#file984516line286>
> >
> > I wonder if we need this comment at all. What it says seems rather obvious. You cannot assign a function value that does not match the given signature.
> >
> > It seems that this comment is a remnant, because a less trivial variant of it follows further down below.
I cleaned this up, see my coments to Niklas below.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review87907
-----------------------------------------------------------
On June 15, 2015, 5:52 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 5:52 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review87907
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment140324>
Could we use some more concrete typing like this here?
std::function<Option<Error>(Option<T>)>&& validate
Similar types could then also be used in all other places where "F&&" appears below.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment140325>
I wonder if we need this comment at all. What it says seems rather obvious. You cannot assign a function value that does not match the given signature.
It seems that this comment is a remnant, because a less trivial variant of it follows further down below.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment140323>
What if T1 is Path? Then fetch() only parses. BTW: what does this mean then? This does not seem to be explained in fetch.hpp or anywhere else.
- Bernd Mathiske
On June 14, 2015, 6:55 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 14, 2015, 6:55 a.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/
-----------------------------------------------------------
(Updated June 14, 2015, 1:55 p.m.)
Review request for mesos.
Changes
-------
Used explicit captures.
Repository: mesos
Description
-------
Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
Diff: https://reviews.apache.org/r/34943/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 11, 2015, 4:43 p.m., Jojy Varghese wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 516
> > <https://reviews.apache.org/r/34943/diff/3/?file=982758#file982758line516>
> >
> > Style comment: Default captures are considered stylistically bad. Captures should be explicit(Meyers, Effective Modern C++, Item 31).
Captured them explicitly here, thanks for the nudge!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review87573
-----------------------------------------------------------
On June 14, 2015, 1:55 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 14, 2015, 1:55 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review87573
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment139961>
Style comment: Default captures are considered stylistically bad. Captures should be explicit(Meyers, Effective Modern C++, Item 31).
- Jojy Varghese
On June 11, 2015, 1:52 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 1:52 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 134453e91d39fd086baa9396ab42a002fed8b73e
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 59cde89b5e035807a510b331b85a8cd48da36ae3
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 4485e41feb8320d2ec7251b8b894ce437f61addd
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp cfe6d742560f50079fb1ed7346526d432615613c
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
> 3rdparty/libprocess/include/Makefile.am d01880c66ca544d62d3ada1ea79c8045884858da
> 3rdparty/libprocess/include/process/firewall.hpp 16ed852d07344e53c6f9d30d4cd7d99e07c6606d
> 3rdparty/libprocess/include/process/process.hpp e70dd388601bd26fcc82f66e0cb95924da8a8c2f
> 3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198
> 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058
> Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15
> docs/configuration.md aaf65bfa2848318c8d07c772ba2c521aa72c2677
> docs/engineering-principles-and-practices.md 6babb929ead758ee5187d5fc5760d084876c3298
> docs/home.md ee3d3e9a417d75af776dad3ea9627d3c6d780dca
> docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e
> docs/powered-by-mesos.md 0ee763ffe32e6667e4ba708dd275d9f1b5116815
> include/mesos/containerizer/containerizer.proto f16ccc89f83da28c413ccfa0687a06b7515a605c
> include/mesos/master/allocator.hpp 92de1af414321281b00eaa6f129e5e3e2c849448
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> src/Makefile.am 10b190288425d735b116ed2dd3f040e871c5cb18
> src/cli/mesos-ps ae48c8cde31e90024381ed3e6e22ec3bc96560b6
> src/examples/no_executor_framework.cpp 6c5c7d3121e3a2ea78f66dffdeaecb72cca6293c
> src/files/files.cpp 3cdd38a3c7122bd5e13c8928279d85ab1373a63e
> src/linux/routing/handle.hpp 052c7cc1a967797d245a275d08cc774f627398a5
> src/linux/routing/queueing/fq_codel.hpp 18c53b416718c27045986d939bb85f9fec731ca5
> src/linux/routing/queueing/fq_codel.cpp 446863391d1c6dd2f91f890596927a20ee59a35f
> src/main.dox d5b29cf96b18ba84ac39f1747c4a174a5bfbc2e1
> src/master/allocator/mesos/allocator.hpp 6cfa04650d91a80211cfbc0809236f9438926c78
> src/master/allocator/mesos/hierarchical.hpp c220ea673320d4d06403afa76f0eb9f3b7a4e5eb
> src/master/flags.hpp 55ed3a9ef84a39841305cca2ba6c5df45c1990e1
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp e91e8815471b49d3c649470197d6da3f06de62fb
> src/master/metrics.hpp 3d389e68f22e3d1e00bde0db4e25f897c79a2316
> src/master/metrics.cpp d2489c8897fb2f2f21f021ac5e7a2ada7997ea06
> src/messages/flags.hpp 41be419ba5593a600aa0c6c411210fa4faa829a8
> src/messages/flags.proto 5400c92297f252734789982d21d7117ef4c57a57
> src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
> src/slave/flags.hpp 6c24e56d15881b0e3aeec3d4824842cf57121fc6
> src/slave/flags.cpp 99142fb1cf9d1978df86fdbf612e656b8fe852a6
> src/slave/main.cpp c379243e01919a5ab30bb9dea1b738665ba4e746
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/master_contender_detector_tests.cpp af6f15a8a85f2a292f5a64286aaa986f7dc29eff
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/no_executor_framework_test.sh b5bb111ceb99d4dc836537516de57c1ba0582371
> src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242
> src/tests/routing_tests.cpp 4be9967d38177d9f7def5c0da98d4c4266e7f0b5
> src/tests/script.cpp bcc1fab912410237dfe903d7a36cad9323d625a0
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/
-----------------------------------------------------------
(Updated June 11, 2015, 1:52 p.m.)
Review request for mesos.
Repository: mesos
Description
-------
Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76
3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 134453e91d39fd086baa9396ab42a002fed8b73e
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 59cde89b5e035807a510b331b85a8cd48da36ae3
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 4485e41feb8320d2ec7251b8b894ce437f61addd
3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp cfe6d742560f50079fb1ed7346526d432615613c
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
3rdparty/libprocess/include/Makefile.am d01880c66ca544d62d3ada1ea79c8045884858da
3rdparty/libprocess/include/process/firewall.hpp 16ed852d07344e53c6f9d30d4cd7d99e07c6606d
3rdparty/libprocess/include/process/process.hpp e70dd388601bd26fcc82f66e0cb95924da8a8c2f
3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198
3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058
Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15
docs/configuration.md aaf65bfa2848318c8d07c772ba2c521aa72c2677
docs/engineering-principles-and-practices.md 6babb929ead758ee5187d5fc5760d084876c3298
docs/home.md ee3d3e9a417d75af776dad3ea9627d3c6d780dca
docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e
docs/powered-by-mesos.md 0ee763ffe32e6667e4ba708dd275d9f1b5116815
include/mesos/containerizer/containerizer.proto f16ccc89f83da28c413ccfa0687a06b7515a605c
include/mesos/master/allocator.hpp 92de1af414321281b00eaa6f129e5e3e2c849448
include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
src/Makefile.am 10b190288425d735b116ed2dd3f040e871c5cb18
src/cli/mesos-ps ae48c8cde31e90024381ed3e6e22ec3bc96560b6
src/examples/no_executor_framework.cpp 6c5c7d3121e3a2ea78f66dffdeaecb72cca6293c
src/files/files.cpp 3cdd38a3c7122bd5e13c8928279d85ab1373a63e
src/linux/routing/handle.hpp 052c7cc1a967797d245a275d08cc774f627398a5
src/linux/routing/queueing/fq_codel.hpp 18c53b416718c27045986d939bb85f9fec731ca5
src/linux/routing/queueing/fq_codel.cpp 446863391d1c6dd2f91f890596927a20ee59a35f
src/main.dox d5b29cf96b18ba84ac39f1747c4a174a5bfbc2e1
src/master/allocator/mesos/allocator.hpp 6cfa04650d91a80211cfbc0809236f9438926c78
src/master/allocator/mesos/hierarchical.hpp c220ea673320d4d06403afa76f0eb9f3b7a4e5eb
src/master/flags.hpp 55ed3a9ef84a39841305cca2ba6c5df45c1990e1
src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp e91e8815471b49d3c649470197d6da3f06de62fb
src/master/metrics.hpp 3d389e68f22e3d1e00bde0db4e25f897c79a2316
src/master/metrics.cpp d2489c8897fb2f2f21f021ac5e7a2ada7997ea06
src/messages/flags.hpp 41be419ba5593a600aa0c6c411210fa4faa829a8
src/messages/flags.proto 5400c92297f252734789982d21d7117ef4c57a57
src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
src/slave/flags.hpp 6c24e56d15881b0e3aeec3d4824842cf57121fc6
src/slave/flags.cpp 99142fb1cf9d1978df86fdbf612e656b8fe852a6
src/slave/main.cpp c379243e01919a5ab30bb9dea1b738665ba4e746
src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
src/tests/master_contender_detector_tests.cpp af6f15a8a85f2a292f5a64286aaa986f7dc29eff
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/no_executor_framework_test.sh b5bb111ceb99d4dc836537516de57c1ba0582371
src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242
src/tests/routing_tests.cpp 4be9967d38177d9f7def5c0da98d4c4266e7f0b5
src/tests/script.cpp bcc1fab912410237dfe903d7a36cad9323d625a0
Diff: https://reviews.apache.org/r/34943/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/
-----------------------------------------------------------
(Updated June 5, 2015, 2:48 p.m.)
Review request for mesos.
Repository: mesos
Description (updated)
-------
Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
Diff: https://reviews.apache.org/r/34943/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/
-----------------------------------------------------------
(Updated June 5, 2015, 2:48 p.m.)
Review request for mesos.
Changes
-------
Addressed Marco's comments.
Also C++11 lambda'fied the remaining Flag functions currently implemented via functors.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
Diff: https://reviews.apache.org/r/34943/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 141
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line141>
> >
> > can you please add some documentation to explain what each type (and the relative @param) is?
> > (using Doxygen would be awesome :)
Most definitely, but I'll do that in another review.
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 146
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line146>
> >
> > How do we enforce that F is a function, and not, say, a std::string?
It gets enforced by using 'F'as a function in the body of 'add'. Bottom line, we take any functor here, if we can invoke the apply operator than we're good to go.
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line184>
> >
> > too much choice, IMO - there are (if I counted them right) 8 different `add()` variants to choose from (none of them documented :)
> >
> > It's virtually impossible for folks to figure out which to pick and this leads, I believe, to the current 'code by copying' approach.
> >
> > I would cull it down to no more than 2-3 different options, with some sensible default values.
I completely agree this needs documentation, which I'll take on in a different review. But there are really only 4 variants here:
(1) The flag _is_ a class member.
(A) The flag has a default value (i.e., is _not_ an Option<T>).
(B) The flag does not have a default value (i.e., _is_ an Option<T>).
(2) The flag is _not_ a class member.
(A) The flag has a default value (i.e., is _not_ an Option<T>).
(B) The flag does not have a default value (i.e., _is_ an Option<T>).
All 4 of these options can optionally have a validation function, but since we can't capture the default validation function with the existing 4 functions we have to do the delegating function trick, resulting in 8 functions. Bottom line, this needs to be documented (and if someone has a better suggestion to avoid the delgating function trick I'm all ears).
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
> > <https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line500>
> >
> > so, with your approach, everyone, every time that they add a required flag, will have to, essentially, copy & paste this code.
> >
> > what I had envisioned, was a simpler way (eg, by adding a `bool required = false` arg) that, if true would automatically do this validation step in `FlagsBase`
> >
> > (of course, we could add another `add()` method that has that signature, that just internally implements this and calls one of the other `add()` ones - making it the ninth option :) )
I wasn't trying to solve the "required" flag problem, I was just trying to show an example of a validation function. But I violently agree that this is therefore a horribly bad example that sets a bad precedent, so I've replaced this with a different validation function.
I also agree that we should introduce a simplier way, such as a bool as you suggested (or better an an enumeration). This doesn't completley solve the problem, however, since a flag without a default value will still be an Option and still require one to do 'CHECK_SOME(flags.flag)'. Got any other ideas?
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 514-517
> > <https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line514>
> >
> > I'll admit I'm still a bith hazy about the new { } initializer, but could have this been:
> > ```
> > char* argv[] = { (char*) "/path/to/program",
> > (char*) "--name1=billy joel" };
> > ```
I just copied this code. Let me give this a test and if so I'll cleanup the others too.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
-----------------------------------------------------------
On June 2, 2015, 2:43 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 2:43 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 61a405f225d14acbc38a80d35570426cb05a3d0a
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp a6e8ba943d97ae908122a444332155ebc6c7bb93
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line184>
> >
> > too much choice, IMO - there are (if I counted them right) 8 different `add()` variants to choose from (none of them documented :)
> >
> > It's virtually impossible for folks to figure out which to pick and this leads, I believe, to the current 'code by copying' approach.
> >
> > I would cull it down to no more than 2-3 different options, with some sensible default values.
>
> Benjamin Hindman wrote:
> I completely agree this needs documentation, which I'll take on in a different review. But there are really only 4 variants here:
>
> (1) The flag _is_ a class member.
> (A) The flag has a default value (i.e., is _not_ an Option<T>).
> (B) The flag does not have a default value (i.e., _is_ an Option<T>).
>
> (2) The flag is _not_ a class member.
> (A) The flag has a default value (i.e., is _not_ an Option<T>).
> (B) The flag does not have a default value (i.e., _is_ an Option<T>).
>
> All 4 of these options can optionally have a validation function, but since we can't capture the default validation function with the existing 4 functions we have to do the delegating function trick, resulting in 8 functions. Bottom line, this needs to be documented (and if someone has a better suggestion to avoid the delgating function trick I'm all ears).
Why should the callers care if it has a default value, type-wise?
My thinking was, let's always use an `Option<T>` for the flag; a, possibly `None()`, default value and a `required` bool flag:
```
template<typename T>
void add(Option<T>* option,
const std::string& name,
const std::string& help,
const Option<T>& const default = None(),
bool required = false)
```
and similarly for the class member.
So, all we have are *two* options, and no doubt as to which one to use.
I'm sure I'm missing a ton here, though... especially given that:
```
Option<string> foo;
Option<string> bar;
Option<string> def_foo{"default-foo-val"};
flags.add(&foo, "foo", "this is foo!", def_foo, true);
flags.add1(&bar, "bar", "this is BAR");
```
works as intended, but:
```
flags.add(&foo, "foo", "this is foo!", "default-foo-val", true);
```
causes a compile error (even though, I can totally see an `Option(const T& _t)` constructor that should "just work" (as it very much does for `def_foo`).
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
> > <https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line500>
> >
> > so, with your approach, everyone, every time that they add a required flag, will have to, essentially, copy & paste this code.
> >
> > what I had envisioned, was a simpler way (eg, by adding a `bool required = false` arg) that, if true would automatically do this validation step in `FlagsBase`
> >
> > (of course, we could add another `add()` method that has that signature, that just internally implements this and calls one of the other `add()` ones - making it the ninth option :) )
>
> Benjamin Hindman wrote:
> I wasn't trying to solve the "required" flag problem, I was just trying to show an example of a validation function. But I violently agree that this is therefore a horribly bad example that sets a bad precedent, so I've replaced this with a different validation function.
>
> I also agree that we should introduce a simplier way, such as a bool as you suggested (or better an an enumeration). This doesn't completley solve the problem, however, since a flag without a default value will still be an Option and still require one to do 'CHECK_SOME(flags.flag)'. Got any other ideas?
> horribly bad example
I never said that :)
As mentioned above, a flag should *always* be an `Option<T>` with the difference that, if we had marked it as `required` then we'd be confident that, once loaded, it does have a value (so, no need to `CHECK_SOME()`) or it would have already failed.
For the optional (aka `non-required`) ones (without a default value) obviously they could be `None()` but that is an "expected outcome" and the caller can take whatever action they deem appropriate.
>From the 'client' perspective:
1. I told you it was `required` --> there must be a value (or you'd have failed before getting back to me)
2. I gave you a default value --> there must be a value (possibly the default one)
3. It's neither required, nor has a default --> its absence (`isNone() == true`) has a meaning to me and I'll take appropriate action
4. It's required, and I gave you a default value --> I'm bad at logical thinking ;-)
I would also propose (similarly to the --help case) to add an `onMissingRequired()` "hook" with the default implementation
```
void onMissingRequired(const string& name)
{
exit(EXIT_FAILURE) << usage("Missing required flag --" + name);
}
```
so that the caller can also take some alternative action.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
-----------------------------------------------------------
On June 5, 2015, 2:48 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:48 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 514-517
> > <https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line514>
> >
> > I'll admit I'm still a bith hazy about the new { } initializer, but could have this been:
> > ```
> > char* argv[] = { (char*) "/path/to/program",
> > (char*) "--name1=billy joel" };
> > ```
>
> Benjamin Hindman wrote:
> I just copied this code. Let me give this a test and if so I'll cleanup the others too.
This is definitely cleaner and we should do it this way going forward so I'm going drop it here and follow up with a subsequent review that takes care of this, thanks for pointing it out!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
-----------------------------------------------------------
On June 5, 2015, 2:48 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:48 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 34943: Added validation to flags.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment138142>
can you please add some documentation to explain what each type (and the relative @param) is?
(using Doxygen would be awesome :)
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment138141>
How do we enforce that F is a function, and not, say, a std::string?
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment138143>
too much choice, IMO - there are (if I counted them right) 8 different `add()` variants to choose from (none of them documented :)
It's virtually impossible for folks to figure out which to pick and this leads, I believe, to the current 'code by copying' approach.
I would cull it down to no more than 2-3 different options, with some sensible default values.
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
<https://reviews.apache.org/r/34943/#comment138144>
so, with your approach, everyone, every time that they add a required flag, will have to, essentially, copy & paste this code.
what I had envisioned, was a simpler way (eg, by adding a `bool required = false` arg) that, if true would automatically do this validation step in `FlagsBase`
(of course, we could add another `add()` method that has that signature, that just internally implements this and calls one of the other `add()` ones - making it the ninth option :) )
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
<https://reviews.apache.org/r/34943/#comment138146>
I'll admit I'm still a bith hazy about the new { } initializer, but could have this been:
```
char* argv[] = { (char*) "/path/to/program",
(char*) "--name1=billy joel" };
```
- Marco Massenzio
On June 2, 2015, 2:43 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 2:43 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 61a405f225d14acbc38a80d35570426cb05a3d0a
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp a6e8ba943d97ae908122a444332155ebc6c7bb93
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>