Add more clang-tidy checks

.clang-tidy

@@ -1,14 +1,21 @@
 ---
 Checks: >
   -*,
+  bugprone-shared-ptr-array-mismatch,
   bugprone-sizeof-expression,
+  bugprone-string-constructor,
+  bugprone-suspicious-memory-comparison,
+  bugprone-suspicious-memset-usage,
+  bugprone-suspicious-semicolon,
   bugprone-suspicious-string-compare,
+  bugprone-unique-ptr-array-mismatch,
   bugprone-use-after-move,
   modernize-use-auto,
   modernize-use-equals-default,
   modernize-use-override,
   modernize-use-using,
   performance-for-range-copy,
+  performance-move-const-arg,
   readability-container-contains,
   readability-const-return-type,
   readability-container-size-empty,

cmake/FlowCommands.cmake

@@ -330,6 +330,9 @@ function(add_flow_target)
     set_property(TARGET ${AFT_NAME} PROPERTY SOURCE_FILES ${AFT_SRCS})
     set_property(TARGET ${AFT_NAME} PROPERTY HEADER_FILES ${HEADER_LIST})
     set_property(TARGET ${AFT_NAME} PROPERTY COVERAGE_FILTERS ${AFT_SRCS})
+    if(generated_files)
+      set_source_files_properties(${generated_files} PROPERTIES SKIP_LINTING ON)
+    endif()
 
     add_custom_target(${AFT_NAME}_actors DEPENDS ${generated_files})
     if(TARGET fdboptions AND NOT "${AFT_NAME}" STREQUAL "fdboptions")

contrib/crc32/CMakeLists.txt

@@ -1,4 +1,5 @@
 add_library(crc32 STATIC crc32.S crc32_wrapper.c crc32c.cpp)
+set_target_properties(crc32 PROPERTIES C_CLANG_TIDY "" CXX_CLANG_TIDY "")
 if (CLANG)
 	# This is necessary for clang since the compiler reports that crc32_align is
 	# defined but not used. With -Werror, crc32 will not compile.

contrib/libb64/CMakeLists.txt

@@ -1,2 +1,3 @@
 add_library(libb64 STATIC cdecode.c cencode.c)
+set_target_properties(libb64 PROPERTIES C_CLANG_TIDY "")
 target_include_directories(libb64 PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/include")

contrib/md5/CMakeLists.txt

@@ -1,2 +1,3 @@
 add_library(md5 STATIC md5.c)
+set_target_properties(md5 PROPERTIES C_CLANG_TIDY "")
 target_include_directories(md5 PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/include")

documentation/sphinx/source/clang-tidy.rst

@@ -10,13 +10,13 @@ This guide explains how to run ``clang-tidy`` locally so you can fix issues befo
 What clang-tidy checks
 ======================
 
-FoundationDB enables 13 checks configured in the ``.clang-tidy`` file at the repository root. The
+FoundationDB enables 21 checks configured in the ``.clang-tidy`` file at the repository root. The
 intent is to enable more as we go forward. Here are some example rules:
 
-* **3 Bugprone rules** -- catch potential runtime errors (e.g., ``bugprone-use-after-move``)
+* **9 Bugprone rules** -- catch potential runtime errors (e.g., ``bugprone-use-after-move``, ``bugprone-suspicious-memory-comparison``)
 * **4 Modernize rules** -- encourage modern C++ practices (e.g., ``modernize-use-auto``, ``modernize-use-override``)
-* **1 Performance rule** -- avoid unnecessary copies (``performance-for-range-copy``)
-* **5 Readability rules** -- improve code clarity (e.g., ``readability-container-contains``, ``readability-container-size-empty``)
+* **2 Performance rules** -- avoid unnecessary copies and pointless moves (e.g., ``performance-for-range-copy``, ``performance-move-const-arg``)
+* **6 Readability rules** -- improve code clarity (e.g., ``readability-container-contains``, ``readability-container-size-empty``)
 
 Basic examples of ``clang-tidy`` style and performance improvement changes:
 
@@ -168,7 +168,9 @@ Optional CMake variables:
 Known limitations
 -----------------
 
-**``.actor.cpp`` files cannot be analyzed.** These files use FoundationDB's custom actor compiler syntax (``ACTOR``, ``wait()``, ``state``) that ``clang-tidy`` cannot parse. Exclude them from your diff when running locally:
+**``.actor.cpp`` files cannot be analyzed.** These files use FoundationDB's custom actor compiler syntax (``ACTOR``, ``wait()``, ``state``) that ``clang-tidy`` cannot parse. CMake skips clang-tidy for generated ``.actor.g.cpp`` outputs. Exclude actor inputs from your diff when running locally.
+
+Build-integrated clang-tidy also skips bundled external-library targets, including ``crc32``, ``libb64``, ``md5``, ``libeio``, and ``libcoroutine``.
 
 Quick reference
 ===============

fdbclient/BackupContainerFileSystem.cpp

@@ -196,7 +196,7 @@ public:
 		json_spirit::mValue json;
 		JSONDoc doc(json);
 
-		doc.create("files") = std::move(fileArray);
+		doc.create("files") = fileArray;
 		doc.create("totalBytes") = totalBytes;
 		doc.create("beginVersion") = minVer;
 		doc.create("endVersion") = maxVer;
@@ -1026,7 +1026,7 @@ public:
 		std::vector<std::string> toDelete;
 
 		// Move filenames out of vector then destroy it to save memory
-		for (auto const& f : logs) {
+		for (auto& f : logs) {
 			// We may have cleared the last log file earlier so skip any empty filenames
 			if (!f.fileName.empty()) {
 				toDelete.push_back(std::move(f.fileName));
@@ -1035,7 +1035,7 @@ public:
 		logs.clear();
 
 		// Move filenames out of vector then destroy it to save memory
-		for (auto const& f : ranges) {
+		for (auto& f : ranges) {
 			// The file version must be checked here again because it is likely that expireEndVersion is in the middle
 			// of a log file, in which case after the log and range file listings are done (using the original
 			// expireEndVersion) the expireEndVersion will be moved back slightly to the begin version of the last log
@@ -1046,7 +1046,7 @@ public:
 		}
 		ranges.clear();
 
-		for (auto const& f : desc.snapshots) {
+		for (auto& f : desc.snapshots) {
 			if (f.endVersion < expireEndVersion)
 				toDelete.push_back(std::move(f.fileName));
 		}
@@ -1677,8 +1677,8 @@ Future<std::vector<RangeFile>> BackupContainerFileSystem::listRangeFiles(Version
 	});
 
 	return map(success(oldFiles) && success(newFiles), [=](Void _) {
-		std::vector<RangeFile> results = std::move(newFiles.get());
-		std::vector<RangeFile> oldResults = std::move(oldFiles.get());
+		std::vector<RangeFile> results = newFiles.get();
+		std::vector<RangeFile> oldResults = oldFiles.get();
 		results.insert(
 		    results.end(), std::make_move_iterator(oldResults.begin()), std::make_move_iterator(oldResults.end()));
 		return results;

fdbclient/DataDistributionConfig.cpp

@@ -44,7 +44,7 @@ json_spirit::mValue DDConfiguration::toJSON(RangeConfigMapSnapshot const& config
 			range["begin"] = rv.range().begin.toString();
 			range["end"] = rv.range().end.toString();
 			range["configuration"] = rv.value().toJSON();
-			ranges.push_back(std::move(range));
+			ranges.push_back(range);
 		}
 	}
 

fdbclient/TagThrottle.cpp

@@ -37,7 +37,7 @@ void TagSet::addTag(TransactionTagRef tag) {
 	TransactionTagRef tagRef(arena, tag);
 	auto it = find(tags.begin(), tags.end(), tagRef);
 	if (it == tags.end()) {
-		tags.push_back(std::move(tagRef));
+		tags.push_back(tagRef);
 		bytes += tag.size();
 	}
 }

fdbclient/WriteMap.cpp

@@ -156,7 +156,7 @@ void WriteMap::mutate(KeyRef key, MutationRef::Type operation, ValueRef param, b
 			    writes,
 			    ver,
 			    e.key); // FIXME: Make PTreeImpl::insert do this automatically (see also VersionedMap.h FIXME)
-			PTreeImpl::insert(writes, ver, std::move(e));
+			PTreeImpl::insert(writes, ver, e);
 		}
 	}
 }
@@ -331,7 +331,7 @@ void WriteMap::addConflictRange(KeyRangeRef keys) {
 	}
 
 	for (int i = 0; i < insertions.size(); i++) {
-		PTreeImpl::insert(writes, ver, std::move(insertions[i]));
+		PTreeImpl::insert(writes, ver, insertions[i]);
 	}
 }
 

fdbrpc/CMakeLists.txt

@@ -37,6 +37,7 @@ target_link_libraries(fdbrpc_test PRIVATE "$<LINK_LIBRARY:WHOLE_ARCHIVE,fdbrpc>"
 
 if(COMPILE_EIO)
   add_library(eio STATIC libeio/eio.c)
+  set_target_properties(eio PROPERTIES C_CLANG_TIDY "")
   if(USE_VALGRIND)
     target_link_libraries(eio PUBLIC valgrind)
   endif()
@@ -53,6 +54,7 @@ if(${COROUTINE_IMPL} STREQUAL libcoro)
     list(APPEND CORO_SRCS libcoroutine/context.c)
   endif()
   add_library(coro STATIC ${CORO_SRCS})
+  set_target_properties(coro PROPERTIES C_CLANG_TIDY "")
   target_link_libraries(coro PRIVATE flow)
   target_include_directories(coro PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/libcoroutine)
   if(WIN32)

fdbrpc/IPAllowList.cpp

@@ -180,7 +180,7 @@ IPAddress parseAddr(std::string const& str) {
 
 struct SubNetTest {
 	AuthAllowedSubnet subnet;
-	explicit SubNetTest(AuthAllowedSubnet&& subnet) : subnet(std::move(subnet)) {}
+	explicit SubNetTest(AuthAllowedSubnet&& subnet) : subnet(subnet) {}
 	explicit SubNetTest(AuthAllowedSubnet const& subnet) : subnet(subnet) {}
 	template <bool V4>
 	static SubNetTest randomSubNetImpl() {

fdbserver/clustercontroller/Status.cpp

@@ -605,7 +605,7 @@ struct RolesInfo {
 				rocksdbMetricsObj.setKeyRawNumber("throttled_commits", rocksdbMetrics.getValue("CommitDelayed"));
 				rocksdbMetricsObj.setKeyRawNumber("write_stall_microseconds", rocksdbMetrics.getValue("StallMicros"));
 
-				obj["rocksdb_metrics"] = std::move(rocksdbMetricsObj);
+				obj["rocksdb_metrics"] = rocksdbMetricsObj;
 			}
 
 		} catch (AttributeNotFoundError& e) {
@@ -3444,7 +3444,7 @@ AsyncResult<StatusReply> clusterGetStatus(
 	    delay(deadlineTimeout));
 
 	if (result.index() == 0) {
-		ErrorOr<Void> statusResult = std::get<0>(std::move(result));
+		ErrorOr<Void> statusResult = std::get<0>(result);
 		if (statusResult.isError()) {
 			status_incomplete_reasons.insert(
 			    fmt::format("Status collection threw: {}", statusResult.getError().what()));

fdbserver/commitproxy/CommitProxyServer.cpp

@@ -1807,7 +1807,7 @@ Future<Void> transactionLogging(CommitBatchContext* self) {
 		auto res = co_await race(self->loggingComplete,
 		                         pProxyCommitData->committedVersion.whenAtLeast(self->commitVersion + 1));
 		if (res.index() == 0) {
-			Version ver = std::get<0>(std::move(res));
+			Version ver = std::get<0>(res);
 			if (!SERVER_KNOBS->ENABLE_VERSION_VECTOR_TLOG_UNICAST) {
 				pProxyCommitData->minKnownCommittedVersion = std::max(pProxyCommitData->minKnownCommittedVersion, ver);
 			}

fdbserver/coordinator/OnDemandStore.cpp

@@ -21,7 +21,7 @@
 #include "OnDemandStore.h"
 
 static Future<Void> onErr(Future<Future<Void>> e) {
-	Future<Void> f = co_await std::move(e);
+	Future<Void> f = co_await e;
 	co_await f;
 }
 

fdbserver/core/StorageMetrics.cpp

@@ -725,7 +725,7 @@ Future<Void> waitMetrics(StorageServerMetrics* self, WaitMetricsRequest req, Fut
 			try {
 				auto res = co_await race(change.getFuture(), timeout);
 				if (res.index() == 0) {
-					metrics += std::get<0>(std::move(res));
+					metrics += std::get<0>(res);
 				} else {
 					timedout = true;
 				}

fdbserver/datadistributor/DDTxnProcessor.cpp

@@ -439,7 +439,7 @@ class DDTxnProcessorImpl {
 					for (auto& r : ranges) {
 						ASSERT(!r.value()->valid);
 					}
-					result->dataMoveMap.insert(meta.ranges.front(), std::move(dataMove));
+					result->dataMoveMap.insert(meta.ranges.front(), dataMove);
 					++numDataMoves;
 				}
 

fdbserver/ratekeeper/Ratekeeper.cpp

@@ -1224,7 +1224,7 @@ UpdateCommitCostRequest StorageQueueInfo::refreshCommitCost(double elapsed) {
 	}
 
 	while (!topKWriters.empty()) {
-		busiestWriteTags.push_back(std::move(topKWriters.top()));
+		busiestWriteTags.push_back(topKWriters.top());
 		topKWriters.pop();
 	}
 
@@ -1269,7 +1269,7 @@ TLogQueueInfo::TLogQueueInfo(UID id)
 
 void TLogQueueInfo::update(TLogQueuingMetricsReply const& reply, Smoother& smoothTotalDurableBytes) {
 	valid = true;
-	auto prevReply = std::move(lastReply);
+	auto prevReply = lastReply;
 	lastReply = reply;
 	if (prevReply.instanceID != reply.instanceID) {
 		smoothDurableBytes.reset(reply.bytesDurable);

fdbserver/storageserver/TransactionTagCounter.cpp

@@ -51,7 +51,7 @@ class TransactionTagCounterImpl {
 		}
 		std::vector<BusyTagInfo> result;
 		while (!topKTags.empty()) {
-			result.push_back(std::move(topKTags.top()));
+			result.push_back(topKTags.top());
 			topKTags.pop();
 		}
 		return result;

fdbserver/tlog/TLogServer.cpp

@@ -2626,7 +2626,7 @@ Future<Void> rejoinClusterController(TLogData* self,
 				    co_await race(brokenPromiseToNever(self->dbInfo->get().clusterInterface.tlogRejoin.getReply(req)),
 				                  self->dbInfo->onChange());
 				if (res.index() == 0) {
-					TLogRejoinReply rep = std::get<0>(std::move(res));
+					TLogRejoinReply rep = std::get<0>(res);
 					if (rep.masterIsRecovered)
 						lastMasterLifetime = self->dbInfo->get().masterLifetime;
 				} else {

fdbserver/tlog/TestTLogServer.cpp

@@ -114,7 +114,7 @@ struct StorageResources {
 
 	StorageResources() = default;
 	StorageResources(std::string dq, std::string kv, TempStorageFiles files)
-	  : diskQueueFilename(std::move(dq)), kvStoreFilename(std::move(kv)), tempFiles(std::move(files)) {}
+	  : diskQueueFilename(std::move(dq)), kvStoreFilename(std::move(kv)), tempFiles(files) {}
 };
 
 StorageResources setupPersistentStorage(Reference<TLogContext> tLogContext,
@@ -134,7 +134,7 @@ StorageResources setupPersistentStorage(Reference<TLogContext> tLogContext,
 
 	TempStorageFiles tempFiles(
 	    diskQueueFilename, options.diskQueueExtension, kvStoreFilename, options.kvStoreExtension);
-	return StorageResources(diskQueueFilename, kvStoreFilename, std::move(tempFiles));
+	return StorageResources(diskQueueFilename, kvStoreFilename, tempFiles);
 }
 
 Reference<TLogTestContext> initTLogTestContext(TestTLogOptions tLogOptions,
@@ -234,7 +234,7 @@ Future<Void> getTLogCreateActor(Reference<TLogTestContext> pTLogTestContext,
 	// wait for either test completion or tLog failure.
 	auto choice = co_await race(tl, pTLogContext->TestTLogServerCompleted.getFuture());
 	if (choice.index() == 1) {
-		bool testCompleted = std::get<1>(std::move(choice));
+		bool testCompleted = std::get<1>(choice);
 		ASSERT_EQ(testCompleted, true);
 	}
 

fdbserver/worker/MetricClient.cpp

@@ -135,7 +135,7 @@ void UDPMetricClient::send(MetricCollection* metrics) {
 		for (const auto& msg : metrics->statsd_message) {
 			// Account for max udp packet size (+1 since we add '\n')
 			if (messages.size() + msg.size() + 1 < IUDPSocket::MAX_PACKET_SIZE) {
-				messages += (std::move(msg) + '\n');
+				messages += (msg + '\n');
 			} else {
 				send_packet(socket_fd, buf.buffer.get(), buf.data_size);
 			}

fdbserver/workloads/AutomaticIdempotencyWorkload.cpp

@@ -479,7 +479,7 @@ struct AutomaticIdempotencyWorkload : TestWorkload {
 			    co_await race(testCleanerOneIteration(db, &actors, minAgeSeconds, maxTimestampDelta, &createdTimes),
 			                  actors.getResult());
 			if (choice.index() == 0) {
-				bool done = std::get<0>(std::move(choice));
+				bool done = std::get<0>(choice);
 
 				if (done) {
 					break;

fdbserver/workloads/DcLag.cpp

@@ -173,7 +173,7 @@ struct DcLagWorkload : TestWorkload {
 				// Fetch DC lag every 5s
 				status = fetchDatacenterLag(cx);
 			} else if (choice.index() == 1) {
-				Optional<double> lag = std::get<1>(std::move(choice));
+				Optional<double> lag = std::get<1>(choice);
 
 				if (lag.present() && lag.get() >= SERVER_KNOBS->LOG_ROUTER_PEEK_SWITCH_DC_TIME - 10.0) {
 					// Detect DC Lag happened before Log router switch DC reactions

fdbserver/workloads/FailoverWithSSLag.cpp

@@ -194,7 +194,7 @@ struct FailoverWithSSLagWorkload : TestWorkload {
 				// Fetch SS lag every 5s.
 				ssLag = fetchStorageServerLag(cx);
 			} else if (choice.index() == 1) {
-				Optional<Version> versionLag = std::get<1>(std::move(choice));
+				Optional<Version> versionLag = std::get<1>(choice);
 
 				if (versionLag.present() && versionLag.get() >= SERVER_KNOBS->MAX_VERSION_DIFFERENCE) {
 					TraceEvent("SSLag").detail("Versions", versionLag.get());

fdbserver/workloads/MiniCycle.cpp

@@ -85,7 +85,7 @@ struct MiniCycleWorkload : TestWorkload {
 		while (true) {
 			auto choice = co_await race(self->_checkCycle(cx->clone(), self, ok), end);
 			if (choice.index() == 0) {
-				bool ret = std::get<0>(std::move(choice));
+				bool ret = std::get<0>(choice);
 
 				ok = ret && ok;
 				if (!ok)

fdbserver/workloads/RemoveServersSafely.cpp

@@ -612,7 +612,7 @@ struct RemoveServersSafelyWorkload : TestWorkload {
 				{
 					auto choice = co_await race(checkSafeExclusions(cx, toKillMarkFailedArray), delay(5.0));
 					if (choice.index() == 0) {
-						bool _safe = std::get<0>(std::move(choice));
+						bool _safe = std::get<0>(choice);
 
 						safe = _safe && protectServers(std::set<AddressExclusion>(toKillMarkFailedArray.begin(),
 						                                                          toKillMarkFailedArray.end()))

flow/IndexedSet.cpp

@@ -287,7 +287,7 @@ TEST_CASE("performance/flow/IndexedSet/integers") {
 	double start = timer();
 	for (int i = 0; i < x.size(); i++) {
 		int t = x[i];
-		is.insert(std::move(t), 3);
+		is.insert(t, 3);
 	}
 	double end = timer();
 	double kps = x.size() / 1000.0 / (end - start);

flow/Net2.cpp

@@ -206,7 +206,7 @@ public:
 		if (thread_network == this)
 			stopCallbacks.emplace_back(std::move(fn));
 		else
-			onMainThreadVoid([this, fn] { this->stopCallbacks.emplace_back(std::move(fn)); });
+			onMainThreadVoid([this, fn = std::move(fn)]() mutable { this->stopCallbacks.emplace_back(std::move(fn)); });
 	}
 
 	bool isSimulated() const override { return false; }

flow/ProcessEvents.cpp

@@ -164,7 +164,7 @@ void uncancellableEvent(StringRef name, Callback callback) {
 }
 
 Event::Event(StringRef name, Callback callback) {
-	impl = new EventImpl({ std::move(name) }, std::move(callback));
+	impl = new EventImpl({ name }, std::move(callback));
 }
 Event::Event(std::vector<StringRef> names, Callback callback) {
 	impl = new EventImpl(std::move(names), std::move(callback));
@@ -266,4 +266,4 @@ TEST_CASE("/flow/ProcessEvents") {
 	return Void();
 }
 
-} // namespace ProcessEvents
+} // namespace ProcessEvents

flow/XmlTraceLogFormatter.cpp

@@ -75,7 +75,7 @@ void XmlTraceLogFormatter::escape(std::ostringstream& oss, std::string source) c
 		source = source.substr(index + 1);
 	}
 
-	oss << std::move(source);
+	oss << source;
 }
 
 std::string XmlTraceLogFormatter::formatEvent(const TraceEventFields& fields) const {