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

Warning while building example #532

Closed
octopus-prime opened this issue Jun 23, 2017 · 18 comments
Closed

Warning while building example #532

octopus-prime opened this issue Jun 23, 2017 · 18 comments

Comments

@octopus-prime
Copy link
Contributor

Building this example (using Beast/65)
https://github.com/vinniefalco/Beast/blob/master/example/websocket-client-ssl/websocket_client_ssl.cpp
with
gcc 7.0.1 and -std=c++17
shows this warnings:

gcc.compile.c++ bin/gcc-7/release/example/foo.o

    "g++"  -ftemplate-depth-128 -march=native -ftemplate-depth=256 -std=c++17 -O3 -finline-functions -Wno-inline -Wall -fPIC  -DBOOST_ALL_DYN_LINK -DNDEBUG  -I"/home/mike/workspace/Beast/include" -I"include" -c -o "bin/gcc-7/release/example/foo.o" "example/foo.cpp"

In file included from /home/mike/workspace/Beast/include/beast/zlib/deflate_stream.hpp:41:0,
                 from /home/mike/workspace/Beast/include/beast/websocket/detail/pmd_extension.hpp:14,
                 from /home/mike/workspace/Beast/include/beast/websocket/detail/stream_base.hpp:17,
                 from /home/mike/workspace/Beast/include/beast/websocket/stream.hpp:14,
                 from /home/mike/workspace/Beast/include/beast/websocket.hpp:16,
                 from example/foo.cpp:174:
/home/mike/workspace/Beast/include/beast/zlib/detail/deflate_stream.hpp: In member function ‘void beast::zlib::detail::deflate_stream::doWrite(beast::zlib::z_params&, beast::zlib::Flush, beast::error_code&) [with <template-parameter-1-1> = void]’:
/home/mike/workspace/Beast/include/beast/zlib/detail/deflate_stream.hpp:1056:30: warning: ‘*((void*)& old_flush +4)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     else if(zs.avail_in == 0 && flush <= old_flush &&
             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
gcc.link bin/gcc-7/release/hessian-example
@vinniefalco
Copy link
Member

Yeah I don't really know how to fix that, its a false positive - do you have any suggestions?

@vinniefalco
Copy link
Member

I'm investigating, it does not happen on clang, msvc, or gcc 6:
https://travis-ci.org/vinniefalco/Beast/jobs/246285814

It does happen on gcc 5
https://travis-ci.org/vinniefalco/Beast/jobs/246285811

@octopus-prime
Copy link
Contributor Author

There you have a comparison
flush <= old_flush
comparing
Flush and boost::optional<Flush>

I am not sure and the doc
http://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/tutorial/relational_operators.html
is not that clear about the mixed type comparison.

So the basic idea is to change the type of variable flush to boost::optional<Flush> too.
By changing the signature of doWrite to
doWrite(z_params& zs, boost::optional<Flush> flush, error_code& ec)
and modifying this block

        switch(strategy_)
        {
        case Strategy::huffman:
            bstate = deflate_huff(zs, flush.get());
            break;
        case Strategy::rle:
            bstate = deflate_rle(zs, flush.get());
            break;
        default:
        {
            bstate = (this->*(get_config(level_).func))(zs, flush.get());
            break;
        }
        }

Have a look at the uploaded file. Compiles without warning.

deflate_stream.txt

@vinniefalco
Copy link
Member

Very interesting!

@vinniefalco
Copy link
Member

On gcc 5 it did not fix the problem:
https://travis-ci.org/vinniefalco/Beast/jobs/246301785#L1338

vinniefalco@b834218

@octopus-prime
Copy link
Contributor Author

octopus-prime commented Jun 23, 2017

Tried your workaround with boost::optional<Flush> flush = flush_param.
Get warnings too.

So try my approach. With boost::optional<Flush> flush as argument in doWrite()

@octopus-prime
Copy link
Contributor Author

BTW: Have you inspected boost iostream zlib?
http://www.boost.org/doc/libs/1_64_0/libs/iostreams/doc/classes/zlib.html

@vinniefalco
Copy link
Member

Yes, that requires that you link against zlib. The whole point of the zlib port in Beast is that 1. It is c++11 with type safety, and 2. it is header-only, does not require building a linking a separate lib.

@octopus-prime
Copy link
Contributor Author

octopus-prime commented Jun 23, 2017

But header-only is not true for ssl?!
So using ssl needs linking openssl. ssl is optional.

Perhaps using zlib needs linking zlib? is zlib optional too?

You know... 3000 lines of code. And reinvent the wheel... Or use a lib.

@octopus-prime
Copy link
Contributor Author

But serious. Beast is not header-only at all.

Always requiered

/site-config//boost_system
/site-config//pthread

Optional

/site-config//boost_coroutine
/site-config//crypto
/site-config//ssl

So, who cares about one optional lib more? And for what a price?
boost::iostream decided to not re-implement zlib too but use the optional library dependency to zlib...

But you decided to maintain zlib support by yourself. We will respect it.
And of course the poor windows developers (no apt, no yum, no brew) will love you :-)

@octopus-prime
Copy link
Contributor Author

Did the approach with boost::optional<Flush> flush as argument in doWrite() work?

@vinniefalco
Copy link
Member

vinniefalco commented Jun 23, 2017

And of course the poor windows developers (no apt, no yum, no brew) will love you :-)

I'm one of those Windows developers!

"header-only" means that Beast does not come with a build script for building its library. So in that sense, it is very much header-only. The very first complaint about Boost.Http (a library that was reviewed in 2015) was "this is not header-only." There is a preference for header-only libraries, they are easier to maintain and easier to integrate. So that is why I made those choices.

Do note that Beast's zlib implementation is a port and not a rewrite of zlib.

@octopus-prime
Copy link
Contributor Author

Yes. I thought so.
And I KNOW the pain with 3rd party libs on windows - believe me ;-)
So i always feel poor, if i have to do jobs on windows...
Building 3rd party libs for example via cygwin, configure and make... :-(

Again, your decision to include zlib stuff is very fine!
If i have to use beast on windows i will be very very very happy :-)

@octopus-prime
Copy link
Contributor Author

In less than a month beast is in boost.
So zlib is in boost without dependency.
So your implementation should be in iostreams too.

@vinniefalco
Copy link
Member

@vinniefalco
Copy link
Member

So your implementation should be in iostreams too.

The Beast version of zlib does not support the gzip wrapper, I removed it because WebSocket doesn't need it. I will add it back at some point, but it needs more extensive tests. I don't think iostreams should use the zlib from Beast yet.

@vinniefalco
Copy link
Member

Is this resolved?

@vinniefalco
Copy link
Member

It looks like this has been resolved so I will close the issue - thanks!

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

2 participants