Add and fix clang -Wshift-sign-overflow (#7431)

Summary:
This option is apparently used by some teams within Facebook
(internal ref T75998621)

Pull Request resolved: https://github.com/facebook/rocksdb/pull/7431

Test Plan: USE_CLANG=1 make check before (fails) and after

Reviewed By: jay-zhuang

Differential Revision: D23876584

Pulled By: pdillinger

fbshipit-source-id: abb8b67a1f1aac75327944d266e284b2b6727191
main
Peter Dillinger 4 years ago committed by Facebook GitHub Bot
parent 0698598f3a
commit 31d1cea4f3
  1. 5
      Makefile
  2. 2
      db/write_batch_test.cc
  3. 20
      include/rocksdb/utilities/backupable_db.h

@ -421,6 +421,11 @@ default: all
WARNING_FLAGS = -W -Wextra -Wall -Wsign-compare -Wshadow \ WARNING_FLAGS = -W -Wextra -Wall -Wsign-compare -Wshadow \
-Wunused-parameter -Wunused-parameter
ifdef USE_CLANG
# Used by some teams in Facebook
WARNING_FLAGS += -Wshift-sign-overflow
endif
ifeq ($(PLATFORM), OS_OPENBSD) ifeq ($(PLATFORM), OS_OPENBSD)
WARNING_FLAGS += -Wno-unused-lambda-capture WARNING_FLAGS += -Wno-unused-lambda-capture
endif endif

@ -419,7 +419,7 @@ TEST_F(WriteBatchTest, PrepareCommit) {
TEST_F(WriteBatchTest, DISABLED_ManyUpdates) { TEST_F(WriteBatchTest, DISABLED_ManyUpdates) {
// Insert key and value of 3GB and push total batch size to 12GB. // Insert key and value of 3GB and push total batch size to 12GB.
static const size_t kKeyValueSize = 4u; static const size_t kKeyValueSize = 4u;
static const uint32_t kNumUpdates = uint32_t(3 << 30); static const uint32_t kNumUpdates = uint32_t{3} << 30;
std::string raw(kKeyValueSize, 'A'); std::string raw(kKeyValueSize, 'A');
WriteBatch batch(kNumUpdates * (4 + kKeyValueSize * 2) + 1024u); WriteBatch batch(kNumUpdates * (4 + kKeyValueSize * 2) + 1024u);
char c = 'A'; char c = 'A';

@ -129,7 +129,7 @@ struct BackupableDBOptions {
// table file names when the table files are stored in the shared_checksum // table file names when the table files are stored in the shared_checksum
// directory (i.e., both share_table_files and share_files_with_checksum // directory (i.e., both share_table_files and share_files_with_checksum
// are true). // are true).
enum ShareFilesNaming : int { enum ShareFilesNaming : uint32_t {
// Backup SST filenames are <file_number>_<crc32c>_<file_size>.sst // Backup SST filenames are <file_number>_<crc32c>_<file_size>.sst
// where <crc32c> is an unsigned decimal integer. This is the // where <crc32c> is an unsigned decimal integer. This is the
// original/legacy naming scheme for share_files_with_checksum, // original/legacy naming scheme for share_files_with_checksum,
@ -140,7 +140,7 @@ struct BackupableDBOptions {
// so generally requires reading the whole file even if the file // so generally requires reading the whole file even if the file
// is already backed up. // is already backed up.
// ** ONLY RECOMMENDED FOR PRESERVING OLD BEHAVIOR ** // ** ONLY RECOMMENDED FOR PRESERVING OLD BEHAVIOR **
kLegacyCrc32cAndFileSize = 1, kLegacyCrc32cAndFileSize = 1U,
// Backup SST filenames are <file_number>_s<db_session_id>.sst. This // Backup SST filenames are <file_number>_s<db_session_id>.sst. This
// pair of values should be very strongly unique for a given SST file // pair of values should be very strongly unique for a given SST file
@ -152,9 +152,9 @@ struct BackupableDBOptions {
// will be used instead, matching the names assigned by RocksDB versions // will be used instead, matching the names assigned by RocksDB versions
// not supporting the newer naming scheme. // not supporting the newer naming scheme.
// * See also flags below. // * See also flags below.
kUseDbSessionId = 2, kUseDbSessionId = 2U,
kMaskNoNamingFlags = 0xffff, kMaskNoNamingFlags = 0xffffU,
// If not already part of the naming scheme, insert // If not already part of the naming scheme, insert
// _<file_size> // _<file_size>
@ -165,7 +165,7 @@ struct BackupableDBOptions {
// //
// We do not consider SST file sizes to have sufficient entropy to // We do not consider SST file sizes to have sufficient entropy to
// contribute significantly to naming uniqueness. // contribute significantly to naming uniqueness.
kFlagIncludeFileSize = 1 << 31, kFlagIncludeFileSize = 1U << 31,
// When encountering an SST file from a Facebook-internal early // When encountering an SST file from a Facebook-internal early
// release of 6.12, use the default naming scheme in effect for // release of 6.12, use the default naming scheme in effect for
@ -175,7 +175,7 @@ struct BackupableDBOptions {
// and ignores kFlagIncludeFileSize setting. // and ignores kFlagIncludeFileSize setting.
// NOTE: This flag is intended to be temporary and should be removed // NOTE: This flag is intended to be temporary and should be removed
// in a later release. // in a later release.
kFlagMatchInterimNaming = 1 << 30, kFlagMatchInterimNaming = 1U << 30,
kMaskNamingFlags = ~kMaskNoNamingFlags, kMaskNamingFlags = ~kMaskNoNamingFlags,
}; };
@ -234,8 +234,8 @@ struct BackupableDBOptions {
inline BackupableDBOptions::ShareFilesNaming operator&( inline BackupableDBOptions::ShareFilesNaming operator&(
BackupableDBOptions::ShareFilesNaming lhs, BackupableDBOptions::ShareFilesNaming lhs,
BackupableDBOptions::ShareFilesNaming rhs) { BackupableDBOptions::ShareFilesNaming rhs) {
int l = static_cast<int>(lhs); uint32_t l = static_cast<uint32_t>(lhs);
int r = static_cast<int>(rhs); uint32_t r = static_cast<uint32_t>(rhs);
assert(r == BackupableDBOptions::kMaskNoNamingFlags || assert(r == BackupableDBOptions::kMaskNoNamingFlags ||
(r & BackupableDBOptions::kMaskNoNamingFlags) == 0); (r & BackupableDBOptions::kMaskNoNamingFlags) == 0);
return static_cast<BackupableDBOptions::ShareFilesNaming>(l & r); return static_cast<BackupableDBOptions::ShareFilesNaming>(l & r);
@ -244,8 +244,8 @@ inline BackupableDBOptions::ShareFilesNaming operator&(
inline BackupableDBOptions::ShareFilesNaming operator|( inline BackupableDBOptions::ShareFilesNaming operator|(
BackupableDBOptions::ShareFilesNaming lhs, BackupableDBOptions::ShareFilesNaming lhs,
BackupableDBOptions::ShareFilesNaming rhs) { BackupableDBOptions::ShareFilesNaming rhs) {
int l = static_cast<int>(lhs); uint32_t l = static_cast<uint32_t>(lhs);
int r = static_cast<int>(rhs); uint32_t r = static_cast<uint32_t>(rhs);
assert((r & BackupableDBOptions::kMaskNoNamingFlags) == 0); assert((r & BackupableDBOptions::kMaskNoNamingFlags) == 0);
return static_cast<BackupableDBOptions::ShareFilesNaming>(l | r); return static_cast<BackupableDBOptions::ShareFilesNaming>(l | r);
} }

Loading…
Cancel
Save