mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 69673: Disallowed nan, inf and so on when parsing Value::Scalar.
Date Mon, 07 Jan 2019 04:25:03 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69673/#review211712
-----------------------------------------------------------


Fix it, then Ship it!




Thanks for adding this validation! Looks good, just a suggestion for a clearer error message
when the value is invalid

Can you add a ticket for this?


src/common/values.cpp
Lines 710-715 (patched)
<https://reviews.apache.org/r/69673/#comment297236>

    We probably want to be a bit clearer about what's wrong with it rather than saying what
not wrong is? I don't think the reader would know what "normal" is?
    
    E.g.
    
    ```
        Option<Error> error = [=]() {
          switch(std::fpclassify(*value_)) {
              case FP_NORMAL:    return None();
              case FP_ZERO:      return None();
              case FP_INFINITE:  return Error("Infinite values not supported");
              case FP_NAN:       return Error("NaN not supported)";
              case FP_SUBNORMAL: return Error("Subnormal values not supported");
              default:           return Error("Unknown error");
          }
        }();
        
        if (error.isSome()) {
          return Error("Invalid scalar value '" + temp + "'"
                       ": " + error->message);
        }
    ```
    
    btw we try to open and close the ' on the same line as the quoted item when possible to
make it a bit clearer to the reader that the quoting is correct



src/common/values.cpp
Lines 715-721 (original), 722-728 (patched)
<https://reviews.apache.org/r/69673/#comment297237>

    This case is actually pretty unfortunate.. if the user typos in "3.1a" it will fall into
TEXT :(



src/tests/values_tests.cpp
Lines 93-103 (patched)
<https://reviews.apache.org/r/69673/#comment297238>

    You could group each pair without a newline? Do you also want to add a subnormal case?



src/v1/values.cpp
Lines 738-743 (patched)
<https://reviews.apache.org/r/69673/#comment297239>

    Ditto here.


- Benjamin Mahler


On Jan. 5, 2019, 11:49 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69673/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2019, 11:49 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Scalar values are intended to be finite numbers. This
> patch checks `nan`, `inf` and so on when parsing
> `Value::Scalar`. Only normal or zero numbers (as defined
> in `std::fpclassify()`) are allowed.
> 
> Also added related tests.
> 
> 
> Diffs
> -----
> 
>   src/common/values.cpp f04d115d2651da5a2784cb9e76757ecef9e2c118 
>   src/tests/values_tests.cpp e4fcf982ac385f58e602eca78765f85485194e41 
>   src/v1/values.cpp 1be99459ef8eb427b44f87234d64ebf099cd7866 
> 
> 
> Diff: https://reviews.apache.org/r/69673/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message