Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15387
## Summary
RocksDB's `.clang-tidy` enables `cppcoreguidelines-special-member-functions`,
`cppcoreguidelines-virtual-class-destructor` and the noexcept-move checks,
and clang warns `-Wdeprecated-copy-with-user-provided-dtor`. A full scan
reports 476 findings. In most of them, a class declares a destructor (or
copy operations) but leaves the rest of copy/move implicit. That has two
costs. Readers can't tell whether a class may be copied or moved. And
resource-owning classes keep an implicit copy that would double-free if
anyone ever used it. This diff resolves 453 of the 476.
Approach, picked per class from least to most invasive:
1. An empty user-provided destructor becomes `= default`. This covers most
interface classes, such as `virtual ~X() {}` -> `= default`. If the
class is still copied, that would only rename the deprecated-copy
warning, so the destructor is removed instead when nothing needs it
(`Customizable`, `TableFactory` and `InternalKeyComparator` inherit a
virtual one; `IteratorWrapperBase` isn't polymorphic), and
`MemTableRep::KeyComparator` gets defaulted copy and move.
2. Classes whose copy is already deleted get explicitly deleted move
operations too. Move was already unavailable, so this only documents
it.
3. Resource-owning classes that are never copied get deleted copy and move
operations. Examples: the C API wrapper structs, which run a
`destructor_` callback (edited in `tools/c_api_gen/c_base.cc` and
regenerated into `db/c.cc`); `ShardedCache`, which frees its shards;
iterator wrappers; and RAII handles.
4. Value types that are copied or moved get the missing operations as
`= default`, which keeps their current behavior.
5. Exception specifications of existing functions are unchanged, except
for `WriteBatch`'s move assignment (below): no other `noexcept` is
added or removed. Only the move operations this diff
newly declares (`Comparator`, `FileSampledStats` and the thread-local
`Entry`) are `noexcept`; they copy only scalars or atomics.
`WriteBatch`'s copy and move assignment destroyed `*this` and
placement-new'ed a new `WriteBatch` over it. That is undefined behavior
when `*this` is a subclass, and if the copy constructor threw, `*this`
was left destroyed and was later destroyed again. Move assignment now
assigns member-wise and ends in the same state as the move constructor,
which leaves `has_key_with_ts_`, the timestamp-size tracking and the
other fields it doesn't copy at their defaults. `rep_` is swapped through
a temporary, because a plain string move-assignment can keep the old
buffer (e.g. a large batch reset with `batch = WriteBatch()` would keep
its memory). Nothing in it can throw, so it is now `noexcept`. Copy
assignment copies into a temporary and moves it in, so a throwing copy
leaves `*this` unchanged.
Bugs this exposed and fixed:
- `RestoreAfterCopyOrCreateWorkItem` (backup engine): the hand-written
move constructor and assignment moved only some fields, so `from_file`
and `to_file` were empty after `push_back(std::move(...))`. A restore
that hit a checksum mismatch then reported "While restoring -> : ..."
without file names. It now uses the compiler-generated moves, which
are still `noexcept`.
- `ExternalSstFileIngestionJob` lived in a `std::vector`, and its
destructor frees raw-owned pointers, so the implicit copy used on
vector reallocation would double-free. Its file batches also point into
its own inline file storage, so neither a copy nor a move is safe after
`Prepare()`. Copy and move are now deleted, and `DBImpl` holds jobs
through `std::unique_ptr`.
Compatibility: no public class loses an operation users can call. Public
interfaces mostly only get `virtual ~X() = default;`.
`WriteBatch`'s move assignment becomes `noexcept`, which is not part of
the mangled name, so the ABI doesn't change.
`SubcompactionJobInfo` and `CompactionJobInfo` intentionally stay C++20
aggregates, so users' aggregate and designated initializers keep
compiling. `Configurable`, `CompareInterface`, `WriteBatchBase`,
`ObjectLibrary::Entry` and `MemTableRep::KeyComparator` declare defaulted
copy and move, and `Customizable` no longer declares a destructor. As a
result, `Configurable` and `Customizable` subclasses that declare no
destructor or copy operations of their own now move their
registered-options list instead of copying it. In-tree, these are the
mutable-options adapters, the table factories, the memory allocators and
test classes; all are held through smart pointers or used in place, and
`Clone()` copies, so none is moved by value.
Not changed (23): `IODebugContext` (custom deep copy plus a
`std::shared_mutex`), `DBWithColumnFamilies` in db_bench (stored in a
`std::vector` with non-movable members), `Snapshot`'s protected virtual
destructor (intended), `ManagedSnapshot`, the `SubcompactionJobInfo` and
`CompactionJobInfo` aggregates, and the 16 noexcept-move-constructor and
noexcept-swap findings on existing functions, including `autovector`'s
move assignment (adding `noexcept` would change their exception
specifications). Copying `ManagedSnapshot` already double-releases the
snapshot. Fixing that needs a real move implementation and is a behavior
change, so it is left for a separate diff.
Reviewed By: rban1
Differential Revision: D123186309
fbshipit-source-id: 6f8e57e233e12199a007407130bb9a1f6d52e8d1
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15366
## Summary
RocksDB's `.clang-tidy` enables `modernize-use-override`, and clang
reports `-Winconsistent-missing-destructor-override` when a class marks
its other overrides `override` but not its destructor. A full scan finds
190 `modernize-use-override`, 100
`inconsistent-missing-destructor-override` and 2 `suggest-override`
findings in existing code. Each one comes back whenever a nearby line is
edited. They also leave classes where it isn't obvious which methods are
overrides and which introduce new virtuals.
This diff marks every overriding member function and destructor
`override` and drops the redundant `virtual` on them. It also changes
`final override` to plain `final`, following the check's default. Methods
that introduce a new virtual keep `virtual`. Most edits are clang-tidy's
own fix-its, applied mechanically and then clang-formatted.
Compatibility: no signatures change. In public headers the only effect is
`override` on existing overrides (mostly destructors, plus
`Cache::Name()` and `Cache::GetPrintableOptions()`, whose base
declarations are unconditional), so user code that subclasses these types
is unaffected. Every touched class now marks all of its overrides
consistently. This matters because a partially-marked class would make
clang emit `-Winconsistent-missing-override` in user builds.
Reviewed By: rban1
Differential Revision: D123186308
fbshipit-source-id: 1f489ea32d938e3ec0faf9abc4f7ff49e042d37a
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15325
## Summary
RocksDB's `.clang-tidy` enables the `clang-analyzer-*` and `bugprone-*`
checks. A full scan of the repo still reports 78 findings from these
checks in existing code. Each time a nearby line is edited, they come
back as noise, and a few of them are real defects. This diff resolves 73
of the 78; upstream has since fixed one more, and the four left are
listed under "Intentionally not changed". Some fixes repair real bugs.
The rest make the code's intent explicit to the analyzer without
changing behavior.
Real bugs fixed:
- `port/mmap.cc`: `MemMapping::operator=(MemMapping&&)` ran the
destructor on `*this`, `memcpy`'d over a non-trivially-copyable object,
then placement-new'd the source. That is undefined behavior, and `*this`
can be the base subobject of a `TypedMemMapping`. It now moves the
members one by one, and the previous mapping is released through a
local `MemMapping`.
- `port/mmap.h`: `TypedMemMapping::operator=(MemMapping&&)` had no
`return *this;`, so calling it was undefined behavior.
- `cache/typed_cache.h`: `if constexpr (sizeof(TCreateContext) > 0)` is
always true, so the branch meant for empty create contexts (for example
`BlobContentsCreator`) could never run. The check is now
`!std::is_empty_v<TCreateContext>`. Existing instantiations still call
the same `Create` function.
- `cassandra_row_merge_test.cc`: the test reused a vector after
`std::move`-ing it into `RowValue::Merge(std::vector<RowValue>&&)`. That
only worked because `Merge` happens not to consume its input. The test
now rebuilds the input.
Behavior-preserving changes:
- Virtual calls in constructors and destructors are now explicitly
qualified (for example `DBImpl::CloseImpl()` in `~DBImpl`). During
construction and destruction, dispatch already resolved to these
functions.
- Stale `/*name=*/` argument comments now match the parameter names.
- `unique_ptr::release()` after a successful cache `Insert` (where the
cache takes ownership) uses `[[maybe_unused]] T* released = x.release();`,
the pattern already used in `table_cache.cc`.
- Removed `class Mutex;` and `class StaticMeta;` forward declarations
that sat in the wrong namespace.
- Added default member initializers for the
`TablePropertiesCollectorFactory::Context` fields that its default
constructor left uninitialized.
- Smaller cleanups: a redundant `cond ? 3 : 3` in a test,
`sizeof(std::string)` in a memory estimate, and an explicit
`Cleanable` base move via `up_cast`. Tests that intentionally use
moved-from objects now carry `NOLINT(bugprone-use-after-move)`,
matching the existing convention.
Intentionally not changed: enum casts in tests that check invalid-value
handling; three `clang-analyzer-core.NullDereference` reports the
analyzer can't prove safe (the C API timestamp out-parameters in
`db/c.cc`, which are only written when the caller asked for a
timestamp, and the waiter-list walk in `util/async_file_reader.cc`);
bit-flag enums in public headers (`FileOpenContract`,
`OptionTypeFlags`), where OR-combined values are valid; ownership
transfers the analyzer can't see across translation units
(`partitioned_index_reader.cc`, `fs_posix.cc`); and the intentional
two-separator check in `file/filename.cc`.
Reviewed By: rban1
Differential Revision: D123186307
fbshipit-source-id: 623e871286b1c73263b1309cb194692194199e0f
Summary:
... which is the default for CentOS 9 and Ubuntu 24, the latter of which is now available in GitHub Actions. Relevant CI job updated.
Re-formatted all cc|c|h files except in third-party/, using
```
clang-format -i `git ls-files | grep -E '[.](cc|c|h)$' | grep -v third-party/`
```
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13233
Test Plan: CI
Reviewed By: jaykorean, archang19
Differential Revision: D67461638
Pulled By: pdillinger
fbshipit-source-id: 0c9ac21a3f5eea6f5ade68bb6af7b6ba16c8b301
Summary:
`-Wextra-semi` or `-Wextra-semi-stmt`
If the code compiles, this is safe to land.
Reviewed By: dmm-fb
Differential Revision: D52968964
fbshipit-source-id: 2cb8c683f958742e2f151db8ef6824ab622528e6
Summary:
Appears possible after 5de98f2 introduced possible lost
updates. Could be related to 2af132c also. Simply ensure no sqrt of
negative.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/10586
Test Plan: test added
Reviewed By: ajkr
Differential Revision: D39068391
Pulled By: pdillinger
fbshipit-source-id: 230b214a41e6c9ae91a1ef3e8b2a17b46bbb17c2
Summary:
`valueIndexMap_` in histogram is redundant and search in `valueIndexMap_` is slower than search in `bucketValues_`.
this PR delete `valueIndexMap_` and search in `bucketValues_` by `std::lower_bound`
Pull Request resolved: https://github.com/facebook/rocksdb/pull/8625
Reviewed By: zhichao-cao
Differential Revision: D31613386
Pulled By: ajkr
fbshipit-source-id: d7415d724f5c8f41f80cbe82afd7467cfad6f009
Summary:
When dynamically linking two binaries together, different builds of RocksDB from two sources might cause errors. To provide a tool for user to solve the problem, the RocksDB namespace is changed to a flag which can be overridden in build time.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6433
Test Plan: Build release, all and jtest. Try to build with ROCKSDB_NAMESPACE with another flag.
Differential Revision: D19977691
fbshipit-source-id: aa7f2d0972e1c31d75339ac48478f34f6cfcfb3e
Summary:
The goal is to reduce the number of histogram buckets, particularly now that we print these histograms for each column family. I chose 1.5 as the factor. We can adjust it later to either make buckets more granular or make fewer buckets.
Closes https://github.com/facebook/rocksdb/pull/2139
Differential Revision: D4872076
Pulled By: ajkr
fbshipit-source-id: 87790d782a605506c3d24190a028cecbd7aa564a
Summary:
Move some files under util/ to new directories env/, monitoring/ options/ and cache/
Closes https://github.com/facebook/rocksdb/pull/2090
Differential Revision: D4833681
Pulled By: siying
fbshipit-source-id: 2fd8bef
