Portability fixes and compiler warning fixes #9
Conversation
facebook-github-bot
added
the
CLA Signed
jsquyres
force-pushed
the
pr/fix-compiler-warnings
branch
from
8a48198
to
16aa6aa
Compare
There was a problem hiding this comment.
@lnicco has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Thank you for contributing! Can you remove the changes for fixing unsigned/signed comparisons? As you noted it is in test code, and the comparisons are actually not unsafe. I'm additionally not a fan of the U UL and ULL suffixes as most of mvfst uses fixed length integral types, for which there are not convenient C++ suffixes. Can you instead disable the warning for tests? |
jsquyres
force-pushed
the
pr/fix-compiler-warnings
branch
from
16aa6aa
to
350692b
Compare
|
Done! Took a little bit because I had to learn a bit about cmake. I also added a commit about some |
There was a problem hiding this comment.
@mjoras has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
jsquyres
force-pushed
the
pr/fix-compiler-warnings
branch
from
350692b
to
ce90b7d
Compare
There was a problem hiding this comment.
@mjoras has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
jsquyres
force-pushed
the
pr/fix-compiler-warnings
branch
from
ce90b7d
to
504a9c7
Compare
jsquyres
force-pushed
the
pr/fix-compiler-warnings
branch
3 times, most recently
from
cc04e26
to
5f63e30
Compare
Remove a commented-out line, and remove what looks like a leftover debug message. Signed-off-by: Jeff Squyres <jeff@squyres.com>
Only add -Werror=bool-compare if the compiler actually supports it. Some compilers do not (e.g., MacOS Mojave 10.14 Apple LLVM version 10.0.1 (clang-1001.0.46.4)). Signed-off-by: Jeff Squyres <jeff@squyres.com>
Suppress compiler warnings by marking a sometimes-unused function parameter. Signed-off-by: Jeff Squyres <jeff@squyres.com>
Quiet compiler warnings by adding a _Nonnull qualifier to the return of QuicStreamManager::getStream(). Signed-off-by: Jeff Squyres <jeff@squyres.com>
Disable some compiler warnings that originate from -Wextra in _QUIC_BASE_COMPILE_OPTIONS. E.g., without -Wno-sign-compare, we get oodles of warnings in the tests about comparing unsigned variables with integer constants. Signed-off-by: Jeff Squyres <jeff@squyres.com>
jsquyres
force-pushed
the
pr/fix-compiler-warnings
branch
from
5f63e30
to
ca74a2e
Compare
|
@mjoras Unless you have another suggestion, I don't see a way to avoid the |
There was a problem hiding this comment.
@mjoras has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
CMakeLists.txt-Werror=bool-compareif the compiler actually supports itFOLLY_MAYBE_UNUSEDto avoid compiler warnings