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

Fix a few warnings #128

Merged
merged 3 commits into from
Jun 5, 2018
Merged

Fix a few warnings #128

merged 3 commits into from
Jun 5, 2018

Conversation

krypt-n
Copy link
Contributor

@krypt-n krypt-n commented May 30, 2018

This PR updates rapidjson to 1.1 since 1.0.2 was giving me a lot of warnings on gcc 8.1.0 and clang 6.0.0.

Additionally it adresses #114 by adding the requested braces. This warning is only emitted on clang <= 5.0.0.

With this PR, vroom compiles without warnings for me on gcc 8.1.0 and clang 6.0.0.

Issue

fixes #114

Tasks

  • verify that there are no warnings on clang 3.8.0 or other versions

@jcoupey
Copy link
Collaborator

jcoupey commented Jun 2, 2018

The warning is gone on clang 3.8.0. On the other hand I'm getting a linking error (already happening in master):

//usr/lib/x86_64-linux-gnu/libstdc++.so.6: error adding symbols: DSO missing from command-line

The symbol is _ZNSt15basic_streambufIcSt11char_traitsIcEE7seekoffElSt12_Ios_SeekdirSt13_Ios_Openmode@@GLIBCXX_3.4. Fiddling around, I managed to get rid of it with:

diff --git a/src/makefile b/src/makefile
index c752296..74db6ae 100644
--- a/src/makefile
+++ b/src/makefile
@@ -5,8 +5,8 @@
 
 # Variables.
 CXX ?= g++
-CXXFLAGS = -MMD -MP -I. -std=c++14 -Wextra -Wpedantic -Wall -O3 -DBOOST_LOG_DYN_LINK
-LDLIBS = -lboost_system -lboost_log -lboost_log_setup -lpthread -lboost_thread
+CXXFLAGS = -MMD -MP -I. -std=c++14 -Wextra -Wpedantic -Wall -O3 -DBOOST_LOG_DYN_LINK -stdlib=libstdc++
+LDLIBS = -lboost_system -lboost_log -lboost_log_setup -lpthread -lboost_thread -lstdc++
 
 # Using all cpp files in current directory.
 MAIN = ../bin/vroom

Not sure if this is limited to this clang version, or if we should include those flags for clang only?

@krypt-n
Copy link
Contributor Author

krypt-n commented Jun 2, 2018

Whats you CXX? This looks like your are using clang which is the C-compiler (equivalent to gcc). At least I get the same error if I use clang. clang++ (or g++) are the compilers that are supposed to be used for C++ and automatically take care of linking the C++ standard library.

The flags you added tell the C-compiler to link the C++ standard library.

@jcoupey
Copy link
Collaborator

jcoupey commented Jun 2, 2018

My bad, I'd been indeed trying with clang, everything is fine using clang++...

@jcoupey
Copy link
Collaborator

jcoupey commented Jun 2, 2018

Regarding the rapidjson version, I also had that in mind. I did not know it was generating warnings on more recent compilers, so all the more a good reason to update.

I've updated my rapidjson clone to v1.1.0 but diffing with the local include/rapidjson folder in this PR points out lots of differences. For example we don't have the cursorstreamwrapper.h file and most *.h files have differences. Did you checkout a specific tag or gitsha?

@jcoupey
Copy link
Collaborator

jcoupey commented Jun 2, 2018

Note for future reference: if we upgrade to v1.1, it would be nice to take advantage of the new support for range-based for loops to remove the unnecessary use of rapidjson::SizeType indices whenever possible.

@krypt-n
Copy link
Contributor Author

krypt-n commented Jun 2, 2018

I downloaded this zip: https://github.com/Tencent/rapidjson/archive/v1.1.0.zip and manually copied the files from the include directory. If I take a look at the v.1.1.0 tag via the github webinterface, it does not contain the cursorstreamwrapper.h file.

/e However, I'll check if I included the correct version

@jcoupey
Copy link
Collaborator

jcoupey commented Jun 2, 2018

I looked at the diff the wrong way around, the cursorstreamwrapper.h file seems to have been removed between v1.0.2 and v1.1.0.

This fixes warnings on gcc 8.1.0 and clang 6.0.0, should have no
downsides
No semantic change, silences a warning on clang 3.8.0 through 5.0.0
@krypt-n
Copy link
Contributor Author

krypt-n commented Jun 4, 2018

Yep, looks like I did not use the correct rapidjson version and made a mistake while copying. I updated the commits in this PR such that rapidjson is now the version from the tag v1.1.0 plus 54dab1ee, since that commit silences a new warning that was introduced in gcc 8.

@jcoupey jcoupey merged commit be7f44b into VROOM-Project:master Jun 5, 2018
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

Successfully merging this pull request may close these issues.

Fix clang warning
2 participants