Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remaining -Wconversion warnings #748

Closed
miketsts opened this issue Apr 3, 2020 · 5 comments
Closed

Remaining -Wconversion warnings #748

miketsts opened this issue Apr 3, 2020 · 5 comments

Comments

@miketsts
Copy link
Contributor

miketsts commented Apr 3, 2020

Hi,

After my PR introducing -Wconversion compilation flag a couple of weeks ago, there still remain a couple of warnings that I don't know how to resolve:

In file included from /home/user/pistache/src/common/http_defs.cc:11:0:
/home/user/pistache/include/pistache/date.h: In constructor ‘constexpr date::weekday_indexed::weekday_indexed(const date::weekday&, unsigned int)’:
/home/user/pistache/include/pistache/date.h:1641:11: warning: conversion to ‘unsigned char:4’ from ‘unsigned char’ may alter its value [-Wconversion]
     : wd_(static_cast<decltype(wd_)>(static_cast<unsigned>(wd))),
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/user/pistache/include/pistache/date.h:1642:14: warning: conversion to ‘unsigned char:4’ from ‘unsigned char’ may alter its value [-Wconversion]
       index_(static_cast<decltype(index_)>(index)) {}
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is conversion warning from "unsigned char" to bitfield 'unsigned char:4'. There is already static_cast<decltype(index_)> - apparently this doesn't work. It looks like the following fixes these warnings:

@@ -1638,8 +1638,8 @@ inline bool weekday_indexed::ok() const NOEXCEPT {
 CONSTCD11
 inline weekday_indexed::weekday_indexed(const date::weekday &wd,
                                         unsigned index) NOEXCEPT
-    : wd_(static_cast<decltype(wd_)>(static_cast<unsigned>(wd))),
-      index_(static_cast<decltype(index_)>(index)) {}
+    : wd_(static_cast<unsigned>(wd) & 0xF),
+      index_(index & 0xF) {}

Any comments?

[ 69%] Building CXX object src/CMakeFiles/pistache.dir/common/http.cc.o
In file included from /home/user/pistache/include/pistache/http.h:20:0,
                 from /home/user/pistache/src/common/http.cc:8:
/home/user/pistache/include/pistache/async.h: In instantiation of ‘void Pistache::Async::Private::impl::Continuation<T, Resolve, Reject, Pistache::Async::Promise<U>(Args ...)>::doResolve(const std::shared_ptr<Pistache::Async::Private::CoreT<T> >&) [with T = long int; Resolve = std::function<Pistache::Async::Promise<long int>(int)>; Reject = std::function<void(std::__exception_ptr::exception_ptr&)>; U = long int; Args = {int}]’:
/home/user/pistache/src/common/http.cc:1008:1:   required from here
/home/user/pistache/include/pistache/async.h:465:10: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
     auto promise = resolve_(detail::tryMove<Resolve>(core->value()));
          ^~~~~~~

This one deals with hard core templating in Pistache, and I'm didn't succeed to get to the root.

Thanks a lot

@dennisjenkins75
Copy link
Collaborator

Ignore everything in date.h. We did not write it; it was imported from some other place. I should move it from "include/pistache" into "include/pistache/thirdparty".

I've not dug into the code (this is just a drive-by comment), but I'm surprised about the warning for the "auto promise =". Why did the compiler fail to create the desired type?

Does this warning occur on both GCC-9 and Clang-?

@miketsts
Copy link
Contributor Author

miketsts commented Apr 3, 2020

I run gcc 7, I don't have the other environments, sorry.
And warnings appear in my compilation out, as I compile pistache from source as git submodule. So I can't just ignore them - or we need to do something in CMakeLists.txt.

Actually, it was my commit a couple of weeks ago that added -Wconversion - maybe we need to remove it eventually?

@miketsts
Copy link
Contributor Author

miketsts commented Apr 5, 2020

I found that the following change fixes the warning in async.h:

diff --git a/src/common/http.cc b/src/common/http.cc
index b8728aa..499f352 100644
--- a/src/common/http.cc
+++ b/src/common/http.cc
@@ -794,7 +794,7 @@ Async::Promise<ssize_t> ResponseWriter::putOnWire(const char *data,
     auto fd = peer()->fd();
 
     return transport_->asyncWrite(fd, buffer)
-        .then<std::function<Async::Promise<ssize_t>(int)>,
+        .then<std::function<Async::Promise<ssize_t>(ssize_t)>,
               std::function<void(std::exception_ptr &)>>(
             [=](int /*l*/) {
               return Async::Promise<ssize_t>([=](

I don't know if it's a correct one though.

@miketsts
Copy link
Contributor Author

miketsts commented Apr 5, 2020

The change below fixes the warning happening in date.h. I've checked the latest version of date.h in the source location - they suppress the warning the similar way, only locally in the file itself.

diff --git a/src/common/http_defs.cc b/src/common/http_defs.cc
index 31e3e76..30019cd 100644
--- a/src/common/http_defs.cc
+++ b/src/common/http_defs.cc
@@ -8,7 +8,14 @@
 #include <iostream>
 
 #include <pistache/common.h>
+#ifdef __GNUC__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wconversion"
+#endif
 #include <pistache/date.h>
+#ifdef __GNUC__
+#pragma GCC diagnostic pop
+#endif
 #include <pistache/http_defs.h>
 
 namespace Pistache {

miketsts pushed a commit to miketsts/pistache that referenced this issue Apr 12, 2020
Fix a warning about problematic conversion from 'long int' to 'int'
appearing when compiling http.cc with -wConversion flag
miketsts pushed a commit to miketsts/pistache that referenced this issue Apr 12, 2020
Suppress warnings originating in date.h. The similar suppression appears
in the newer version of date.h appearing in its original repository
https://github.com/HowardHinnant/date
@dennisjenkins75
Copy link
Collaborator

This PR breaks all travis builds. For example: https://travis-ci.org/github/oktal/pistache/jobs/674003067

miketsts pushed a commit to miketsts/pistache that referenced this issue Apr 14, 2020
Suppress warnings originating in date.h. The similar suppression appears
in the newer version of date.h appearing in its original repository
https://github.com/HowardHinnant/date
dennisjenkins75 pushed a commit that referenced this issue May 3, 2020
* Fix -wConversion warning in async.h #748

Fix a warning about problematic conversion from 'long int' to 'int'
appearing when compiling http.cc with -wConversion flag

* Suppress -wConversion warnings in date.h #748

Suppress warnings originating in date.h. The similar suppression appears
in the newer version of date.h appearing in its original repository
https://github.com/HowardHinnant/date

Co-authored-by: Michael Tseitlin <michael.tseitlin@concertio.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants