mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 69673: Disallowed nan, inf and so on when parsing Value::Scalar.
Date Mon, 07 Jan 2019 06:18:15 GMT


> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > 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?

Done. Linked to the patch.


> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > src/common/values.cpp
> > Lines 710-715 (patched)
> > <https://reviews.apache.org/r/69673/diff/2/?file=2117883#file2117883line710>
> >
> >     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

Done. Thanks!


> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > src/common/values.cpp
> > Lines 715-721 (original), 722-728 (patched)
> > <https://reviews.apache.org/r/69673/diff/2/?file=2117883#file2117883line722>
> >
> >     This case is actually pretty unfortunate.. if the user typos in "3.1a" it will
fall into TEXT :(

I wonder do we have any use case for Value::TEXT atm? I guess we do not know... May be we
could make it as deprecated in the proto and just throw an error here?


> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > src/tests/values_tests.cpp
> > Lines 93-103 (patched)
> > <https://reviews.apache.org/r/69673/diff/2/?file=2117884#file2117884line93>
> >
> >     You could group each pair without a newline? Do you also want to add a subnormal
case?

Done.


- Meng


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


On Jan. 6, 2019, 10:18 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69673/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2019, 10:18 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9510
>     https://issues.apache.org/jira/browse/MESOS-9510
> 
> 
> 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/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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