----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65322/#review196239 -----------------------------------------------------------
Fix it, then Ship it! 3rdparty/stout/include/stout/numify.hpp Lines 34 (patched) <https://reviews.apache.org/r/65322/#comment275787> Its not your comment, but this point sounds like a red herring to me: We're always parsing strings here, so the it shouldn't matter what the rules for permissible floating-point literals are. 3rdparty/stout/include/stout/numify.hpp Lines 43 (patched) <https://reviews.apache.org/r/65322/#comment275784> Why 'maybe'? I'd suggest ``` bool isHex = strings::startsWith(s, "0x") || ...; ``` 3rdparty/stout/include/stout/numify.hpp Line 32 (original), 55 (patched) <https://reviews.apache.org/r/65322/#comment275788> Strictly speaking, since boost is just using `std::istream::operator>>` for numbers anyways, we could just not use boost here at all and just use the code in the `try`-block for everything. 3rdparty/stout/include/stout/numify.hpp Line 53 (original), 65 (patched) <https://reviews.apache.org/r/65322/#comment275786> Not too important, but it took me a while to understand what this note is trying to say. Would suggest changing to something like: ``` Negating `result` is safe even if T is an unsigned integer type, because the C++ standard defines the result to be modulo 2^n in that case. ``` - Benno Evers On Jan. 25, 2018, 6:32 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65322/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2018, 6:32 a.m.) > > > Review request for mesos, Benno Evers, Till Toenshoff, and Cong Wang. > > > Bugs: MESOS-8484 > https://issues.apache.org/jira/browse/MESOS-8484 > > > Repository: mesos > > > Description > ------- > > Previously we relied on Boost to never successfully parse hexadecimal > numbers, and only performed rejection of hexadecimal floating point > numbers before entering our custom parser for hexadecimal numbers. > This was not reliable as on some platforms `boost::lexical_cast` ended > up successfully parsing literals of hexadecimal floating point numbers > leading us to never rejecting hexadecimal floating point numbers. > > This patch moves the rejection of hexadecimal floating point numbers > to a point before attempting a parse with `boost::lexical_cast`. While > this decouples us in some regards from Boost's parsing behavior, we > also might end up performing more work than needed. Should this become > a concern we should revisit and optimize the implementation here. > > > Diffs > ----- > > 3rdparty/stout/include/stout/numify.hpp > 6db9a78145b7e90cc975786ca83f7acb7fdc3e0a > > > Diff: https://reviews.apache.org/r/65322/diff/1/ > > > Testing > ------- > > `make check` on a collection of Linux, macos and Windows setups in internal > CI. > > > Thanks, > > Benjamin Bannier > >
