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.

Sign up for GitHub

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

Jump to bottom

Remove unused MSVC compiler warning supressions #12205

Closed
wants to merge 1 commit into from
Closed

Remove unused MSVC compiler warning supressions #12205

wants to merge 1 commit into from

Conversation

rhubner
Copy link
Contributor

@rhubner rhubner commented Jan 4, 2024 •
edited by ajkr

Remove unused compiler warning supressions as was suggested in #10745.

Fixes #10745

@rhubner rhubner mentioned this pull request Jan 4, 2024
Closed
@rhubner rhubner marked this pull request as ready for review January 4, 2024 19:37
ajkr
ajkr previously approved these changes Jan 4, 2024
View reviewed changes
@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@adamretter adamretter self-requested a review January 5, 2024 13:27
adamretter
adamretter reviewed Jan 5, 2024
View reviewed changes
Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhubner thanks looks good so far, let's see if we can knock out some of the others too.

@ajkr We are going to spend a few days more on this to see if we can remove the remaining ignored warnings...

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

4 similar comments
@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@ajkr ajkr dismissed their stale review January 9, 2024 17:28

outdated

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ajkr
ajkr reviewed Jan 18, 2024
View reviewed changes
db/db_test_util.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

2 similar comments
@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

pdillinger
pdillinger requested changes Mar 12, 2024
View reviewed changes
util/ribbon_impl.h Outdated
@@ -569,10 +569,17 @@ class StandardBanding : public StandardHasher<TypesAndSettings> {
template <typename InputIterator>
bool AddRange(InputIterator begin, InputIterator end) {
assert(num_starts_ > 0 || TypesAndSettings::kAllowZeroStarts);
#if defined(_MSC_VER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just exempting the whole file?

util/math.h Outdated
return static_cast<T>(_byteswap_uint64(static_cast<uint64_t>(v)));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional compilation is easier to reason about with whole statements (or declarations or expressions). Sometimes called "syntactic confinement." IMHO early return is a lesser evil.

db/db_write_test.cc Outdated
@@ -21,6 +21,10 @@
#include "utilities/fault_injection_env.h"
#include "utilities/fault_injection_fs.h"

#ifdef OS_WIN
#define sleep(x) Sleep(x)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about port_win.h?

db/db_test_util.cc Outdated
@@ -352,7 +352,18 @@ Options DBTestBase::GetOptions(
"NewWritableFile:O_DIRECT");
#endif
// kMustFreeHeapAllocations -> indicates ASAN build

#if defined(__has_feature)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be the ugliest hack job I've seen to work around a compiler warning.

You could wrap the whole if ( in

// (Work around MSVC compiler warning)
#if !defined(_MSC_VER) || defined(MUST_FREE_HEAP_ALLOCATIONS)

Or better use the pragma warning disable recipe

util/ribbon_test.cc Outdated
ROCKSDB_GTEST_BYPASS("Not fully supported");
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this is a clear case of early return improving readability by reducing indent depth and apparent complexity. (You want to add indent to 400+ lines of code for this?)

Or does the 4127 warning preclude unreachable code after a constexpr early return? It sounds like a very bad warning. What's wrong with having it disabled globally? If you don't want it disabled in the build why not disable semi-globally in port_win.h or lang.h?

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@rhubner
Copy link
Contributor Author

rhubner commented Mar 14, 2024

@pdillinger thanks for comments. I removed all the /4127 changes and left this PR as simple removal of three warnings. We can remove other warning supressin one-by-one later with individual PRs.
I also borrowed some fixes from your PR
Radek

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@pdillinger
Copy link
Contributor

I also borrowed some fixes from your PR

FWIW, I want those fixes to be committed atomically with fixing the Windows CI. Then we can look at tightening the warnings.

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@rhubner has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in 088dc72.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamretter adamretter adamretter left review comments

@pdillinger pdillinger pdillinger requested changes

@ajkr ajkr ajkr approved these changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable compiler warnings on MS Windows builds
5 participants
@rhubner @facebook-github-bot @pdillinger @adamretter @ajkr