Re: Review Request 56591: Stout: Removed MSVC compiler warnings.

2017-02-16 Thread Joseph Wu 

---
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.

2017-02-15 Thread Alex Clemmer 

---
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.

2017-02-15 Thread Alex Clemmer 


> On Feb. 15, 2017, 2:07 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/duration.hpp, lines 99-100
> > 
> >
> > 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
> > 
> >
> > 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::max());
> > ```
> > ^ Possibly need a `static_cast` 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.

2017-02-14 Thread Joseph Wu 

---
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)


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)


See: https://reviews.apache.org/r/53708/#comment229180



3rdparty/stout/include/stout/gzip.hpp (line 126)


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)


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)


I can squash this change into  https://reviews.apache.org/r/55547/ before 
committing.



3rdparty/stout/include/stout/protobuf.hpp (lines 178 - 180)


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::max());
```
^ Possibly need a `static_cast` in the first argument above.



3rdparty/stout/include/stout/protobuf.hpp (lines 280 - 282)


Ditto with the CHECK_LE.



3rdparty/stout/include/stout/windows/os.hpp (lines 56 - 57)


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.

2017-02-14 Thread Alex Clemmer 

---
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

Review Request 56591: Stout: Removed MSVC compiler warnings.

2017-02-13 Thread Alex Clemmer 

---
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