You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Patrick Reilly <pa...@gmail.com> on 2014/08/26 23:38:48 UTC
Review Request 25079: Replaced macro expansion with variadic template
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/
-----------------------------------------------------------
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1734
https://issues.apache.org/jira/browse/MESOS-1734
Repository: mesos-git
Description
-------
Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
Diffs
-----
3rdparty/libprocess/Makefile.am edbe54b
3rdparty/libprocess/include/process/help.hpp 4333b5b
3rdparty/libprocess/src/help.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/25079/diff/
Testing
-------
Ran "make check".
Thanks,
Patrick Reilly
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Aug. 26, 2014, 7:36 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> >
> > Reviews applied: [25079]
> >
> > Failed command: ./support/mesos-style.py
> >
> > Error:
> > Checking 506 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
> > 3rdparty/libprocess/include/process/help.hpp:61: public: should not be indented inside class TLineHelper [whitespace/indent] [3]
> > 3rdparty/libprocess/include/process/help.hpp:72: public: should not be indented inside class TLineHelper [whitespace/indent] [3]
> > 3rdparty/libprocess/src/help.cpp:77: Tab found; better to use spaces [whitespace/tab] [1]
> > 3rdparty/libprocess/src/help.cpp:78: Tab found; better to use spaces [whitespace/tab] [1]
> > 3rdparty/libprocess/src/help.cpp:124: Tab found; better to use spaces [whitespace/tab] [1]
> > 3rdparty/libprocess/src/help.cpp:136: Tab found; better to use spaces [whitespace/tab] [1]
> > 3rdparty/libprocess/src/help.cpp:139: Tab found; better to use spaces [whitespace/tab] [1]
> > 3rdparty/libprocess/src/help.cpp:154: Tab found; better to use spaces [whitespace/tab] [1]
> > Total errors found: 8
Probably not you guys, but can you address these style bugs too? You can run ./support/mesos-style.py to get the style report.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51633
-----------------------------------------------------------
On Aug. 26, 2014, 2:44 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2014, 2:44 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51633
-----------------------------------------------------------
Bad patch!
Reviews applied: [25079]
Failed command: ./support/mesos-style.py
Error:
Checking 506 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
3rdparty/libprocess/include/process/help.hpp:61: public: should not be indented inside class TLineHelper [whitespace/indent] [3]
3rdparty/libprocess/include/process/help.hpp:72: public: should not be indented inside class TLineHelper [whitespace/indent] [3]
3rdparty/libprocess/src/help.cpp:77: Tab found; better to use spaces [whitespace/tab] [1]
3rdparty/libprocess/src/help.cpp:78: Tab found; better to use spaces [whitespace/tab] [1]
3rdparty/libprocess/src/help.cpp:124: Tab found; better to use spaces [whitespace/tab] [1]
3rdparty/libprocess/src/help.cpp:136: Tab found; better to use spaces [whitespace/tab] [1]
3rdparty/libprocess/src/help.cpp:139: Tab found; better to use spaces [whitespace/tab] [1]
3rdparty/libprocess/src/help.cpp:154: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 8
- Mesos ReviewBot
On Aug. 26, 2014, 9:44 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2014, 9:44 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review53196
-----------------------------------------------------------
Ship it!
This is awesome! I will commit shortly: I fixed a few minor style nits (removing a duplicate comment and adding newlines).
- Niklas Nielsen
On Aug. 28, 2014, 9:24 a.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 9:24 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51938
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25079]
All tests passed.
- Mesos ReviewBot
On Aug. 28, 2014, 4:24 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 4:24 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Cody Maloney <co...@mesosphere.io>.
> On Sept. 12, 2014, 7:03 p.m., Ben Mahler wrote:
> > Thanks for doing this! A few higher level comments:
> >
> > (1) We have strings::join in stout. Have you considered implementing this TLineHelper as a generic Joiner in stout/strings.hpp? Seems like it belongs there instead of a newline joiner in this file. Let's also be sure to not expose it in the higher level namespace.
> >
> > (2) We tend to leverage support/post-reviews.py to break changes apart, for example, the addition of the variadic template and the breaking up of help into a .cpp file could have been done in two separate reviews to speed up the review / commit process. We could get the latter change committed quickly! As much as possible, breaking changes apart into a chain of commits will make your life easier. :)
> >
> > (3) From the discussion on MESOS-750, it seems that gcc 4.4 is the minimum supported compiler version. FWICT, this means that within mesos we can certainly assume variadic templates. My hunch is that we don't want to continue writing both C++11/non-C++11 code paths in stout/libprocess due to the complexity and the maintenance burden, but that's a decision we should seek benh's input on. For now, the #ifdef is certainly a safe way to go.
(1) There are actually a number string joiners throughout the codebase, and lots of unnecessary constant string -> std::string -> appending. Things like the newline adder add a lot of extra compile time while not actually giving less typing than people inserting '\n' by hand (On top of the runtime behavior doing less unnecessary computation)...
(3) I've worked with variadic templates on gcc 4.6 - 4.9, and each until about 4.7 has slight changes in behavior / bugfixes. We really need a bot moving into C++11 land to guarantee that all the compilers we say we support work.
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review53198
-----------------------------------------------------------
On Aug. 28, 2014, 4:24 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 4:24 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Sept. 12, 2014, 7:03 p.m., Ben Mahler wrote:
> > Thanks for doing this! A few higher level comments:
> >
> > (1) We have strings::join in stout. Have you considered implementing this TLineHelper as a generic Joiner in stout/strings.hpp? Seems like it belongs there instead of a newline joiner in this file. Let's also be sure to not expose it in the higher level namespace.
> >
> > (2) We tend to leverage support/post-reviews.py to break changes apart, for example, the addition of the variadic template and the breaking up of help into a .cpp file could have been done in two separate reviews to speed up the review / commit process. We could get the latter change committed quickly! As much as possible, breaking changes apart into a chain of commits will make your life easier. :)
> >
> > (3) From the discussion on MESOS-750, it seems that gcc 4.4 is the minimum supported compiler version. FWICT, this means that within mesos we can certainly assume variadic templates. My hunch is that we don't want to continue writing both C++11/non-C++11 code paths in stout/libprocess due to the complexity and the maintenance burden, but that's a decision we should seek benh's input on. For now, the #ifdef is certainly a safe way to go.
>
> Cody Maloney wrote:
> (1) There are actually a number string joiners throughout the codebase, and lots of unnecessary constant string -> std::string -> appending. Things like the newline adder add a lot of extra compile time while not actually giving less typing than people inserting '\n' by hand (On top of the runtime behavior doing less unnecessary computation)...
>
> (3) I've worked with variadic templates on gcc 4.6 - 4.9, and each until about 4.7 has slight changes in behavior / bugfixes. We really need a bot moving into C++11 land to guarantee that all the compilers we say we support work.
>
> Ben Mahler wrote:
> (1) Good point, we can make strings::join variadic for C++11 and it can do the job for us in help.hpp, to avoid further proliferation of string joiners. :)
>
> (3) Ok, let's keep the #ifdef for now!
I have submitted r25789 and will modify this patch to use the strings::join provided from there.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review53198
-----------------------------------------------------------
On Aug. 28, 2014, 4:24 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 4:24 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 12, 2014, 7:03 p.m., Ben Mahler wrote:
> > Thanks for doing this! A few higher level comments:
> >
> > (1) We have strings::join in stout. Have you considered implementing this TLineHelper as a generic Joiner in stout/strings.hpp? Seems like it belongs there instead of a newline joiner in this file. Let's also be sure to not expose it in the higher level namespace.
> >
> > (2) We tend to leverage support/post-reviews.py to break changes apart, for example, the addition of the variadic template and the breaking up of help into a .cpp file could have been done in two separate reviews to speed up the review / commit process. We could get the latter change committed quickly! As much as possible, breaking changes apart into a chain of commits will make your life easier. :)
> >
> > (3) From the discussion on MESOS-750, it seems that gcc 4.4 is the minimum supported compiler version. FWICT, this means that within mesos we can certainly assume variadic templates. My hunch is that we don't want to continue writing both C++11/non-C++11 code paths in stout/libprocess due to the complexity and the maintenance burden, but that's a decision we should seek benh's input on. For now, the #ifdef is certainly a safe way to go.
>
> Cody Maloney wrote:
> (1) There are actually a number string joiners throughout the codebase, and lots of unnecessary constant string -> std::string -> appending. Things like the newline adder add a lot of extra compile time while not actually giving less typing than people inserting '\n' by hand (On top of the runtime behavior doing less unnecessary computation)...
>
> (3) I've worked with variadic templates on gcc 4.6 - 4.9, and each until about 4.7 has slight changes in behavior / bugfixes. We really need a bot moving into C++11 land to guarantee that all the compilers we say we support work.
(1) Good point, we can make strings::join variadic for C++11 and it can do the job for us in help.hpp, to avoid further proliferation of string joiners. :)
(3) Ok, let's keep the #ifdef for now!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review53198
-----------------------------------------------------------
On Aug. 28, 2014, 4:24 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 4:24 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review53198
-----------------------------------------------------------
Thanks for doing this! A few higher level comments:
(1) We have strings::join in stout. Have you considered implementing this TLineHelper as a generic Joiner in stout/strings.hpp? Seems like it belongs there instead of a newline joiner in this file. Let's also be sure to not expose it in the higher level namespace.
(2) We tend to leverage support/post-reviews.py to break changes apart, for example, the addition of the variadic template and the breaking up of help into a .cpp file could have been done in two separate reviews to speed up the review / commit process. We could get the latter change committed quickly! As much as possible, breaking changes apart into a chain of commits will make your life easier. :)
(3) From the discussion on MESOS-750, it seems that gcc 4.4 is the minimum supported compiler version. FWICT, this means that within mesos we can certainly assume variadic templates. My hunch is that we don't want to continue writing both C++11/non-C++11 code paths in stout/libprocess due to the complexity and the maintenance burden, but that's a decision we should seek benh's input on. For now, the #ifdef is certainly a safe way to go.
- Ben Mahler
On Aug. 28, 2014, 4:24 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 4:24 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Patrick Reilly <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/
-----------------------------------------------------------
(Updated Aug. 28, 2014, 4:24 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Changes
-------
Adding Apache license block
Bugs: MESOS-1734
https://issues.apache.org/jira/browse/MESOS-1734
Repository: mesos-git
Description
-------
Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am edbe54b
3rdparty/libprocess/include/process/help.hpp 4333b5b
3rdparty/libprocess/src/help.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/25079/diff/
Testing
-------
Ran "make check".
Thanks,
Patrick Reilly
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51748
-----------------------------------------------------------
I'm no expert in variadic templates, but the rest of the code (moving help impls into cpp) looks great.
Somebody with more C++11 skillz should take a look.
3rdparty/libprocess/src/help.cpp
<https://reviews.apache.org/r/25079/#comment90324>
Please add Apache license block.
- Adam B
On Aug. 27, 2014, 12:57 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2014, 12:57 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Patrick Reilly <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/
-----------------------------------------------------------
(Updated Aug. 27, 2014, 7:57 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Changes
-------
Fixing compatibility by adding guard against C++11 features with ifdef.
Bugs: MESOS-1734
https://issues.apache.org/jira/browse/MESOS-1734
Repository: mesos-git
Description
-------
Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am edbe54b
3rdparty/libprocess/include/process/help.hpp 4333b5b
3rdparty/libprocess/src/help.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/25079/diff/
Testing
-------
Ran "make check".
Thanks,
Patrick Reilly
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Patrick Reilly <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/
-----------------------------------------------------------
(Updated Aug. 27, 2014, 6:10 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1734
https://issues.apache.org/jira/browse/MESOS-1734
Repository: mesos-git
Description
-------
Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am edbe54b
3rdparty/libprocess/include/process/help.hpp 4333b5b
3rdparty/libprocess/src/help.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/25079/diff/
Testing
-------
Ran "make check".
Thanks,
Patrick Reilly
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51666
-----------------------------------------------------------
Bad patch!
Reviews applied: [25079]
Failed command: ./support/mesos-style.py
Error:
Checking 506 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
3rdparty/libprocess/include/process/help.hpp:61: public: should not be indented inside class TLineHelper [whitespace/indent] [3]
3rdparty/libprocess/include/process/help.hpp:72: public: should not be indented inside class TLineHelper [whitespace/indent] [3]
Total errors found: 2
- Mesos ReviewBot
On Aug. 27, 2014, 5:20 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2014, 5:20 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Patrick Reilly <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/
-----------------------------------------------------------
(Updated Aug. 27, 2014, 5:20 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1734
https://issues.apache.org/jira/browse/MESOS-1734
Repository: mesos-git
Description
-------
Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am edbe54b
3rdparty/libprocess/include/process/help.hpp 4333b5b
3rdparty/libprocess/src/help.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/25079/diff/
Testing
-------
Ran "make check".
Thanks,
Patrick Reilly
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Patrick Reilly <pa...@gmail.com>.
> On Aug. 26, 2014, 9:58 p.m., Ben Mahler wrote:
> > It was done using macro expansion because variadic templates require C++11.
> >
> > We're not yet able to assume C++11: https://issues.apache.org/jira/browse/MESOS-750
>
> Niklas Nielsen wrote:
> But as this reduce compile time for folks with C++11 compilers, can we make this guarded by #ifdef and enable it for good when we make the transition?
>
> Michael Park wrote:
> +1. It seems like we can handle this version by adding it to the c++11 directory and guarding it with #ifdef similar to defer.hpp.
Michael Park please make that change. I guess we will just check with: __cplusplus >= 201103L
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51595
-----------------------------------------------------------
On Aug. 27, 2014, 6:10 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2014, 6:10 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Aug. 26, 2014, 2:58 p.m., Ben Mahler wrote:
> > It was done using macro expansion because variadic templates require C++11.
> >
> > We're not yet able to assume C++11: https://issues.apache.org/jira/browse/MESOS-750
But as this reduce compile time for folks with C++11 compilers, can we make this guarded by #ifdef and enable it for good when we make the transition?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51595
-----------------------------------------------------------
On Aug. 26, 2014, 2:44 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2014, 2:44 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Michael Park <mc...@gmail.com>.
> On Aug. 26, 2014, 5:58 p.m., Ben Mahler wrote:
> > It was done using macro expansion because variadic templates require C++11.
> >
> > We're not yet able to assume C++11: https://issues.apache.org/jira/browse/MESOS-750
>
> Niklas Nielsen wrote:
> But as this reduce compile time for folks with C++11 compilers, can we make this guarded by #ifdef and enable it for good when we make the transition?
+1. It seems like we can handle this version by adding it to the c++11 directory and guarding it with #ifdef similar to defer.hpp.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51595
-----------------------------------------------------------
On Aug. 26, 2014, 5:44 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2014, 5:44 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/#review51595
-----------------------------------------------------------
It was done using macro expansion because variadic templates require C++11.
We're not yet able to assume C++11: https://issues.apache.org/jira/browse/MESOS-750
- Ben Mahler
On Aug. 26, 2014, 9:44 p.m., Patrick Reilly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25079/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2014, 9:44 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1734
> https://issues.apache.org/jira/browse/MESOS-1734
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am edbe54b
> 3rdparty/libprocess/include/process/help.hpp 4333b5b
> 3rdparty/libprocess/src/help.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25079/diff/
>
>
> Testing
> -------
>
> Ran "make check".
>
>
> Thanks,
>
> Patrick Reilly
>
>
Re: Review Request 25079: Replaced macro expansion with variadic
template
Posted by Patrick Reilly <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25079/
-----------------------------------------------------------
(Updated Aug. 26, 2014, 9:44 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1734
https://issues.apache.org/jira/browse/MESOS-1734
Repository: mesos-git
Description
-------
Reduce compile time: - replacing a macro expansion with a variadic template - moving implementation from help.hpp to help.cpp
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am edbe54b
3rdparty/libprocess/include/process/help.hpp 4333b5b
3rdparty/libprocess/src/help.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/25079/diff/
Testing
-------
Ran "make check".
Thanks,
Patrick Reilly