You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2017/02/13 10:21:33 UTC
Review Request 56591: Stout: Removed MSVC compiler warnings.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56591/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Repository: mesos
Description
-------
Stout: Removed MSVC compiler warnings.
Diffs
-----
3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4
3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932
3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4
3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99
3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa
3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea
3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34
Diff: https://reviews.apache.org/r/56591/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56591/#review165791
-----------------------------------------------------------
Ship it!
- Joseph Wu
On Feb. 15, 2017, 1:36 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56591/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 1:36 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Removed MSVC compiler warnings.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4
> 3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932
> 3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4
> 3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99
> 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
> 3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa
> 3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea
> 3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34
>
> Diff: https://reviews.apache.org/r/56591/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56591/
-----------------------------------------------------------
(Updated Feb. 15, 2017, 9:36 p.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Address Joseph's comments.
Repository: mesos
Description
-------
Stout: Removed MSVC compiler warnings.
Diffs (updated)
-----
3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4
3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932
3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4
3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99
3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa
3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea
3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34
Diff: https://reviews.apache.org/r/56591/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Feb. 15, 2017, 2:07 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/duration.hpp, lines 99-100
> > <https://reviews.apache.org/r/56591/diff/2/?file=1633052#file1633052line99>
> >
> > See: https://reviews.apache.org/r/53708/#comment229180
Note: There is a typo, I believe, in that suggested code. In this line:
```
t.tv_usec = (us() / MICROSECONDS) - (t.tv_sec * MILLISECONDS);
```
I think you want to suggest `ns() / MICROSECONDS` instead of `us() / MICROSECONDS`.
Let's also add a comment explaining why we're computing these quantities manually.
> On Feb. 15, 2017, 2:07 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp, lines 178-180
> > <https://reviews.apache.org/r/56591/diff/2/?file=1633057#file1633057line178>
> >
> > Hm...
> >
> > This is a `size_t` (unsigned int) to `int` implicit cast. This means, if the string is of size 2^31 or greater, we will be passing in a negative size to the constructor.
> >
> > In proto2, the maximum size of a protobuf is 64 MB, so we're unlikely to hit the upper limit.
> >
> > However, since it is possible to pass in an arbitrary string to this method, we should add:
> >
> > ```
> > CHECK_LE(value.size(), std::numeric_limits<size_t>::max());
> > ```
> > ^ Possibly need a `static_cast<long>` in the first argument above.
Alright, good idea. We originally didn't do this because I figured it's safe for the reason you mentioned, but I agree that we should be extra cautious.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56591/#review165596
-----------------------------------------------------------
On Feb. 14, 2017, 9:05 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56591/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 9:05 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Removed MSVC compiler warnings.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4
> 3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932
> 3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4
> 3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99
> 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
> 3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa
> 3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea
> 3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34
>
> Diff: https://reviews.apache.org/r/56591/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56591/#review165596
-----------------------------------------------------------
3rdparty/stout/include/stout/abort.hpp (lines 45 - 54)
<https://reviews.apache.org/r/56591/#comment237456>
I'd expand the comment with:
`strlen` always returns a positive result, which means it is safe to cast it to an unsigned value.
3rdparty/stout/include/stout/duration.hpp (lines 99 - 100)
<https://reviews.apache.org/r/56591/#comment237483>
See: https://reviews.apache.org/r/53708/#comment229180
3rdparty/stout/include/stout/gzip.hpp (line 126)
<https://reviews.apache.org/r/56591/#comment237482>
Note to self: The changes to this file supercede the changes (+ review comments) from Daniel's review: https://reviews.apache.org/r/53709/
3rdparty/stout/include/stout/os/windows/dup.hpp (lines 41 - 43)
<https://reviews.apache.org/r/56591/#comment237489>
Do you want to reference/create a JIRA like https://issues.apache.org/jira/browse/MESOS-6817 to track unicode?
3rdparty/stout/include/stout/os/windows/environment.hpp (line 28)
<https://reviews.apache.org/r/56591/#comment237491>
I can squash this change into https://reviews.apache.org/r/55547/ before committing.
3rdparty/stout/include/stout/protobuf.hpp (lines 178 - 180)
<https://reviews.apache.org/r/56591/#comment237487>
Hm...
This is a `size_t` (unsigned int) to `int` implicit cast. This means, if the string is of size 2^31 or greater, we will be passing in a negative size to the constructor.
In proto2, the maximum size of a protobuf is 64 MB, so we're unlikely to hit the upper limit.
However, since it is possible to pass in an arbitrary string to this method, we should add:
```
CHECK_LE(value.size(), std::numeric_limits<size_t>::max());
```
^ Possibly need a `static_cast<long>` in the first argument above.
3rdparty/stout/include/stout/protobuf.hpp (lines 280 - 282)
<https://reviews.apache.org/r/56591/#comment237488>
Ditto with the CHECK_LE.
3rdparty/stout/include/stout/windows/os.hpp (lines 56 - 57)
<https://reviews.apache.org/r/56591/#comment237492>
Ditto: question about tracking unicode with a JIRA. Two locations in this file.
- Joseph Wu
On Feb. 14, 2017, 1:05 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56591/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 1:05 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Removed MSVC compiler warnings.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4
> 3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932
> 3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4
> 3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99
> 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
> 3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa
> 3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea
> 3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34
>
> Diff: https://reviews.apache.org/r/56591/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56591/
-----------------------------------------------------------
(Updated Feb. 14, 2017, 9:05 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Removed unnecessary comments.
Repository: mesos
Description
-------
Stout: Removed MSVC compiler warnings.
Diffs (updated)
-----
3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4
3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932
3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4
3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99
3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa
3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea
3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34
Diff: https://reviews.apache.org/r/56591/diff/
Testing
-------
Thanks,
Alex Clemmer