Re: Review Request 39211: Check for trailing characters in JSON::parse().

2015-10-21 Thread Adam B

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

Ship it!


Committing..


3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 757)


Okay, I finally did the math too, and this checks out now. Tricky 
off-by-ones and pointer arithmetic. If this was anything more than an error 
message, I'd ask you to break this into a couple of intermediate variable 
assignments with comments.


- Adam B


On Oct. 20, 2015, 10:13 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> ---
> 
> (Updated Oct. 20, 2015, 10:13 a.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check for trailing characters in JSON::parse().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> 0870300c2d372722b4850155cb2a8e848b986ed5 
>   3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
> b95667f4c1a1c170016da024a5e4af02ccaa064e 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
> 131d39b87e571f16c774a133eb3148e98d8b9dd4 
> 
> Diff: https://reviews.apache.org/r/39211/diff/
> 
> 
> Testing
> ---
> 
> Added tests to make sure that JSON::parse() is successfully returning errors 
> when passed invalid JSON objects. Ran `make check`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39211: Check for trailing characters in JSON::parse().

2015-10-20 Thread Greg Mann


> On Oct. 20, 2015, 7:28 a.m., Adam B wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, lines 744-748
> > 
> >
> > I'm getting confused between `last_char` and `end`, since `end` isn't 
> > really the `end`. Also, we don't use camel_case. How about: 
> > `lastVisibleChar` and `parseEnd`? Maybe `parseBegin` too?

Yea good call, I like your suggested names. Thanks!


- Greg


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


On Oct. 20, 2015, 4:22 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> ---
> 
> (Updated Oct. 20, 2015, 4:22 p.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check for trailing characters in JSON::parse().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> 0870300c2d372722b4850155cb2a8e848b986ed5 
>   3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
> b95667f4c1a1c170016da024a5e4af02ccaa064e 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
> 131d39b87e571f16c774a133eb3148e98d8b9dd4 
> 
> Diff: https://reviews.apache.org/r/39211/diff/
> 
> 
> Testing
> ---
> 
> Added tests to make sure that JSON::parse() is successfully returning errors 
> when passed invalid JSON objects. Ran `make check`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39211: Check for trailing characters in JSON::parse().

2015-10-20 Thread Greg Mann

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

(Updated Oct. 20, 2015, 4:22 p.m.)


Review request for mesos, Adam B and Joseph Wu.


Changes
---

Addressed comments.


Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698


Repository: mesos


Description
---

Check for trailing characters in JSON::parse().


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
0870300c2d372722b4850155cb2a8e848b986ed5 
  3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
b95667f4c1a1c170016da024a5e4af02ccaa064e 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
131d39b87e571f16c774a133eb3148e98d8b9dd4 

Diff: https://reviews.apache.org/r/39211/diff/


Testing
---

Added tests to make sure that JSON::parse() is successfully returning errors 
when passed invalid JSON objects. Ran `make check`.


Thanks,

Greg Mann



Re: Review Request 39211: Check for trailing characters in JSON::parse().

2015-10-13 Thread Greg Mann

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

(Updated Oct. 13, 2015, 5:51 p.m.)


Review request for mesos, Adam B and Joseph Wu.


Changes
---

Addressed comments.


Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698


Repository: mesos


Description
---

Check for trailing characters in JSON::parse().


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
0870300c2d372722b4850155cb2a8e848b986ed5 
  3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
b95667f4c1a1c170016da024a5e4af02ccaa064e 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
131d39b87e571f16c774a133eb3148e98d8b9dd4 

Diff: https://reviews.apache.org/r/39211/diff/


Testing
---

Added tests to make sure that JSON::parse() is successfully returning errors 
when passed invalid JSON objects. Ran `make check`.


Thanks,

Greg Mann



Re: Review Request 39211: Check for trailing characters in JSON::parse().

2015-10-12 Thread Joseph Wu

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 741)


Nit: s/picojson/PicoJson/

(For consistency with other comments about PicoJson)



3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 754)


Nit: We don't put periods in error messages.


- Joseph Wu


On Oct. 10, 2015, 8:05 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> ---
> 
> (Updated Oct. 10, 2015, 8:05 p.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check for trailing characters in JSON::parse().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> 0870300c2d372722b4850155cb2a8e848b986ed5 
>   3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
> b95667f4c1a1c170016da024a5e4af02ccaa064e 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
> 131d39b87e571f16c774a133eb3148e98d8b9dd4 
> 
> Diff: https://reviews.apache.org/r/39211/diff/
> 
> 
> Testing
> ---
> 
> Added tests to make sure that JSON::parse() is successfully returning errors 
> when passed invalid JSON objects. Ran `make check`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39211: Check for trailing characters in JSON::parse().

2015-10-12 Thread Greg Mann

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

(Updated Oct. 12, 2015, 6:29 p.m.)


Review request for mesos, Adam B and Joseph Wu.


Changes
---

Addressed comments.


Bugs: MESOS-3698
https://issues.apache.org/jira/browse/MESOS-3698


Repository: mesos


Description
---

Check for trailing characters in JSON::parse().


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
0870300c2d372722b4850155cb2a8e848b986ed5 
  3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
b95667f4c1a1c170016da024a5e4af02ccaa064e 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
131d39b87e571f16c774a133eb3148e98d8b9dd4 

Diff: https://reviews.apache.org/r/39211/diff/


Testing
---

Added tests to make sure that JSON::parse() is successfully returning errors 
when passed invalid JSON objects. Ran `make check`.


Thanks,

Greg Mann



Re: Review Request 39211: Check for trailing characters in JSON::parse().

2015-10-12 Thread Adam B

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


Looks great! Just a couple of suggestions to expand unit testing & logging.


3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 754)


Worth including s.substring(end..last_char) in the error message, so the 
user knows what trailing chars?



3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp (line 232)


Could also test the positive case that we correctly parse (no error) a 
jsonString with a valid element and only whitespace trailing.



3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp (line 252)


But parsing this as an array should succeed without error, correct? Can we 
validate that in the test too?


- Adam B


On Oct. 12, 2015, 11:29 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> ---
> 
> (Updated Oct. 12, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check for trailing characters in JSON::parse().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> 0870300c2d372722b4850155cb2a8e848b986ed5 
>   3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
> b95667f4c1a1c170016da024a5e4af02ccaa064e 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
> 131d39b87e571f16c774a133eb3148e98d8b9dd4 
> 
> Diff: https://reviews.apache.org/r/39211/diff/
> 
> 
> Testing
> ---
> 
> Added tests to make sure that JSON::parse() is successfully returning errors 
> when passed invalid JSON objects. Ran `make check`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>