> On May 19, 2017, 8:51 p.m., Neil Conway wrote: > > `stout`, `libprocess`, and `mesos` are nominally distinct projects, so a > > single RR should touch no more than one of them. i.e., please split the > > `stout` changes into a separate review. > > > > Might also be worth waiting to see if GCC upstream is going to fix the > > problem promptly. If GCC 7.2 fixes the problem, we don't necessarily need > > to workaround a bug that occurs in just a single minor release of GCC. > > Aaron Wood wrote: > Sure, I can hold on this and see what they say. If it's something that > still needs to be fixed I'll break out this RR into separate ones.
I am not a big fan of the derived classes of `Bytes` since they are practically designed to be used with slicing (`Bytes b = Kilobytes(42)`) which is nasty by itself. I believe the intention here was to provide easy to use factory functions, and don't believe introducing dedicated types for multiples is a very useful but instead confusing (think e.g., number of implicit conversions and overload resolution). This fix makes sense regardless of whether GCC fixes this particular regression. > On May 19, 2017, 8:51 p.m., Neil Conway wrote: > > src/master/constants.hpp > > Line 49 (original), 49 (patched) > > <https://reviews.apache.org/r/59413/diff/1/?file=1725380#file1725380line49> > > > > This change (and all the similar changes) seems unfortunate. Can't we > > play a similar trick to the change you made to `Bytes`? > > Aaron Wood wrote: > I had wanted to but I didn't see a quick and easy way to do this without > making some major changes. The extended classes in `duration.hpp` mostly look > something like this: > ``` > class Nanoseconds : public Duration > { > public: > explicit constexpr Nanoseconds(int64_t nanoseconds) > : Duration(nanoseconds, NANOSECONDS) {} > > constexpr Nanoseconds(const Duration& d) : Duration(d) {} > > double value() const { return static_cast<double>(this->ns()); } > > static std::string units() { return "ns"; } > }; > ``` > I'm open to ideas, I just figured if I changed these clases a lot I'd > have even more code to change around Mesos. It looks like the only custom method here is `units` which seems to be used by just the lengthy stringification function below. I believe getting rid of the derived classes here makes just as much sense as it did for `Bytes` and friends. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59413/#review175555 ----------------------------------------------------------- On May 19, 2017, 9 p.m., Aaron Wood wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59413/ > ----------------------------------------------------------- > > (Updated May 19, 2017, 9 p.m.) > > > Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and > Neil Conway. > > > Bugs: MESOS-7520 > https://issues.apache.org/jira/browse/MESOS-7520 > > > Repository: mesos > > > Description > ------- > > Many of the `constexpr` variables fail to compile with errors such as `error: > 'Megabytes(32).Megabytes::<anonymous>' is not a constant expression because > it refers to an incompletely initialized variable`. > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80829 > > This patch changes around the `Bytes` class a bit by getting rid of the small > classes that extend `Bytes`. Additionally, any usage of the `Duration` class > has been adjusted to not instantiate using the base type (which triggers the > issue along with `constexpr`). > > > Diffs > ----- > > 3rdparty/stout/include/stout/bytes.hpp 98223db50 > src/master/constants.hpp 725680b1e > src/sched/constants.hpp 9edb25b38 > src/sched/sched.cpp ef73c1dcc > src/scheduler/constants.hpp e3da12646 > src/slave/constants.hpp 9c1d7245c > src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 929b7e0b7 > src/slave/slave.cpp 14de72fa4 > src/slave/status_update_manager.cpp 0cd88ac3a > > > Diff: https://reviews.apache.org/r/59413/diff/3/ > > > Testing > ------- > > `./bootstrap && mkdir build && cd build && ../configure --disable-python > --disable-java --disable-optimize --disable-hardening && make -j$(nproc)` > > > Thanks, > > Aaron Wood > >
