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/14 18:40:58 UTC
Review Request 56675: Silence MSVC compiler warnings in libmesos.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56675/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Repository: mesos
Description
-------
Silence MSVC compiler warnings in libmesos.
Diffs
-----
src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee
src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3
src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a
src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
src/master/allocator/sorter/drf/sorter.cpp ae49fcd659972f984238f8f90aa395e345bfaaa6
src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd
src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163
src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3
src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72
src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef
src/slave/containerizer/mesos/isolators/posix.hpp 627004663cbd7223a252b875c51454a20e645be6
src/slave/containerizer/mesos/launcher.cpp 5114c130efbfb252dde1e85c081f5174e66f57af
src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382
src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5
src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc
src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f
src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59
src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da
src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba
src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
Diff: https://reviews.apache.org/r/56675/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 56675: Silence MSVC compiler warnings in libmesos.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56675/#review165563
-----------------------------------------------------------
I am not sure what other solutions you explored, but I believe a preferable solution would be to produce reasonable types from the start instead of `static_cast`'ing at some use sites (that looks like an indication of somewhat uncoordinated interfaces).
Also, this looks like a rather ad-hoc workaround specific to MSVC's default warnings. I believe a more maintainable solution would be to instead either disable that warning in MSVC (if at all possible), or enable that warning or a similar one for other compilers as well. That would somewhat limit the possibilities of developers primarily on e.g., Linux to introduce similar issues for the MSVC again in the future.
src/tests/resources_tests.cpp (line 68)
<https://reviews.apache.org/r/56675/#comment237422>
This does change semantics since neither 45.55f nor 45.55 are exactly representable in IEEE-754 form, and `EXPECT_*_EQ` considers numbers within 4ULPs as equal. We should pick some representable number here instead.
Could you follow up with a cleanup?
Here and below.
- Benjamin Bannier
On Feb. 14, 2017, 7:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56675/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 7:40 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Silence MSVC compiler warnings in libmesos.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee
> src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3
> src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/master/allocator/sorter/drf/sorter.cpp ae49fcd659972f984238f8f90aa395e345bfaaa6
> src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd
> src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163
> src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3
> src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72
> src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef
> src/slave/containerizer/mesos/isolators/posix.hpp 627004663cbd7223a252b875c51454a20e645be6
> src/slave/containerizer/mesos/launcher.cpp 5114c130efbfb252dde1e85c081f5174e66f57af
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382
> src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5
> src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc
> src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f
> src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59
> src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da
> src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
> Diff: https://reviews.apache.org/r/56675/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56675: Silence MSVC compiler warnings in libmesos.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Feb. 22, 2017, 12:53 a.m., Joseph Wu wrote:
> > I'm going to review/commit this patch in a piece-meal fashion, as some of these warnings expose actual edge cases, while others can be committed directly (or with slight tweaks).
+1
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56675/#review166017
-----------------------------------------------------------
On Feb. 14, 2017, 6:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56675/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 6:40 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Silence MSVC compiler warnings in libmesos.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee
> src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3
> src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/master/allocator/sorter/drf/sorter.cpp ae49fcd659972f984238f8f90aa395e345bfaaa6
> src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd
> src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163
> src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3
> src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72
> src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef
> src/slave/containerizer/mesos/isolators/posix.hpp 627004663cbd7223a252b875c51454a20e645be6
> src/slave/containerizer/mesos/launcher.cpp 5114c130efbfb252dde1e85c081f5174e66f57af
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382
> src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5
> src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc
> src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f
> src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59
> src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da
> src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
> Diff: https://reviews.apache.org/r/56675/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56675: Silence MSVC compiler warnings in libmesos.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Feb. 21, 2017, 4:53 p.m., Joseph Wu wrote:
> > I'm going to review/commit this patch in a piece-meal fashion, as some of these warnings expose actual edge cases, while others can be committed directly (or with slight tweaks).
>
> Andrew Schwartzmeyer wrote:
> +1
Pushed two of these tweaks:
commit fac4c59600d7c1b45fda128b114ebdff25b91a4c
Author: Alex Clemmer <cl...@gmail.com>
Date: Fri Feb 17 18:10:21 2017 -0800
Dealt with some type casting warnings in common/values.cpp.
This fixes the type of the local variable we use to store an
IO stream's original precision while outputing a `Scalar` value.
We extraneously cast from a `std::streamsize` (`int`) to `long`.
Also, this removes an extranous cast from `size_t` to `int` in
a vector iteration loop.
Review: https://reviews.apache.org/r/56675/
commit 039343ab8be125d6491e061d879f42dd9518f9ec
Author: Alex Clemmer <cl...@gmail.com>
Date: Fri Feb 17 17:58:29 2017 -0800
Windows: Silenced protobuf compilation warnings.
Generated code from protobufs generate warnings on MSVC and other
compilers. On MSVC, this clutters the final list of warnings with
many (mostly harmless) warnings that we cannot address, due to
the fact that protobuf code is generated code.
Review: https://reviews.apache.org/r/56675/
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56675/#review166017
-----------------------------------------------------------
On Feb. 14, 2017, 10:40 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56675/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:40 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Silence MSVC compiler warnings in libmesos.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee
> src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3
> src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/master/allocator/sorter/drf/sorter.cpp ae49fcd659972f984238f8f90aa395e345bfaaa6
> src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd
> src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163
> src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3
> src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72
> src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef
> src/slave/containerizer/mesos/isolators/posix.hpp 627004663cbd7223a252b875c51454a20e645be6
> src/slave/containerizer/mesos/launcher.cpp 5114c130efbfb252dde1e85c081f5174e66f57af
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382
> src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5
> src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc
> src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f
> src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59
> src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da
> src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
> Diff: https://reviews.apache.org/r/56675/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56675: Silence MSVC compiler warnings in libmesos.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56675/#review166017
-----------------------------------------------------------
I'm going to review/commit this patch in a piece-meal fashion, as some of these warnings expose actual edge cases, while others can be committed directly (or with slight tweaks).
src/CMakeLists.txt (lines 128 - 134)
<https://reviews.apache.org/r/56675/#comment238191>
This one looks good. I can pull it out and commit it with a fleshed out commit description.
src/common/values.cpp (lines 76 - 77)
<https://reviews.apache.org/r/56675/#comment237908>
This is actually an `streamsize`. Instead of casting, we could just use the correct type.
src/common/values.cpp (lines 610 - 618)
<https://reviews.apache.org/r/56675/#comment237910>
This is some very odd casting... (and apparently it originates from the original code mirrored from the SVN repo by BenH...)
We could just get rid of the `j` variable.
src/files/files.cpp (line 665)
<https://reviews.apache.org/r/56675/#comment238184>
I'm going to create a separate patch chain to see if this is worth a wider fix.
At the moment, if you try to pass in an offset/length like `9223372036854775808` (2^63), depending on the code path, you'll get different results.
For the `/files/read?` codepath, you get:
```
Failed to parse offset: Failed to convert '9223372036854775808' to number.
```
For the V1 API, this query would return a 200, with no data.
All code paths should reject values that large.
Continued in: https://reviews.apache.org/r/56906/
- Joseph Wu
On Feb. 14, 2017, 10:40 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56675/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:40 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Silence MSVC compiler warnings in libmesos.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee
> src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3
> src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/master/allocator/sorter/drf/sorter.cpp ae49fcd659972f984238f8f90aa395e345bfaaa6
> src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd
> src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163
> src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3
> src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72
> src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef
> src/slave/containerizer/mesos/isolators/posix.hpp 627004663cbd7223a252b875c51454a20e645be6
> src/slave/containerizer/mesos/launcher.cpp 5114c130efbfb252dde1e85c081f5174e66f57af
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382
> src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5
> src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc
> src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f
> src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59
> src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da
> src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
> Diff: https://reviews.apache.org/r/56675/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56675: Silence MSVC compiler warnings in libmesos.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56675/#review165586
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55022, 55023, 55024, 55030, 55162, 55543, 55546, 55547, 55549, 55550, 55749, 56504, 56505, 56591, 56592, 56593, 56594, 56652, 56675]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 14, 2017, 6:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56675/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 6:40 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Silence MSVC compiler warnings in libmesos.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee
> src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3
> src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/master/allocator/sorter/drf/sorter.cpp ae49fcd659972f984238f8f90aa395e345bfaaa6
> src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd
> src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163
> src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3
> src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72
> src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef
> src/slave/containerizer/mesos/isolators/posix.hpp 627004663cbd7223a252b875c51454a20e645be6
> src/slave/containerizer/mesos/launcher.cpp 5114c130efbfb252dde1e85c081f5174e66f57af
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382
> src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5
> src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc
> src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f
> src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59
> src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da
> src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
> Diff: https://reviews.apache.org/r/56675/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>