From cb5b851ff8cd2d3b2e8d15c449cd015d87e73bee Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 7 Sep 2021 21:18:21 -0700 Subject: [PATCH] Add (& fix) some simple source code checks (#8821) Summary: * Don't hardcode namespace rocksdb (use ROCKSDB_NAMESPACE) * Don't #include (use double quotes) * Support putting NOCOMMIT (any case) in source code that should not be committed/pushed in current state. These will be run with `make check` and in GitHub actions Pull Request resolved: https://github.com/facebook/rocksdb/pull/8821 Test Plan: existing tests, manually try out new checks Reviewed By: zhichao-cao Differential Revision: D30791726 Pulled By: pdillinger fbshipit-source-id: 399c883f312be24d9e55c58951d4013e18429d92 --- .github/workflows/sanity_check.yml | 3 +++ Makefile | 8 ++++-- build_tools/check-sources.sh | 28 +++++++++++++++++++++ cache/cache_bench.cc | 2 +- db_stress_tool/db_stress.cc | 2 +- examples/compaction_filter_example.cc | 8 +++--- include/rocksdb/memtablerep.h | 3 ++- include/rocksdb/system_clock.h | 5 ++-- java/rocksjni/event_listener_jnicallback.cc | 4 +-- java/rocksjni/event_listener_jnicallback.h | 4 +-- memory/memkind_kmem_allocator.cc | 4 +-- memory/memkind_kmem_allocator.h | 4 +-- memory/memkind_kmem_allocator_test.cc | 4 +-- table/block_based/index_builder.cc | 3 +-- test_util/testharness.h | 2 +- tools/db_bench.cc | 2 +- 16 files changed, 61 insertions(+), 25 deletions(-) create mode 100755 build_tools/check-sources.sh diff --git a/.github/workflows/sanity_check.yml b/.github/workflows/sanity_check.yml index 276851155..e6a5f1591 100644 --- a/.github/workflows/sanity_check.yml +++ b/.github/workflows/sanity_check.yml @@ -39,3 +39,6 @@ jobs: - name: Compare buckify output run: make check-buck-targets + + - name: Simple source code checks + run: make check-sources diff --git a/Makefile b/Makefile index e2790a2cc..b37e43e54 100644 --- a/Makefile +++ b/Makefile @@ -943,6 +943,7 @@ endif ifndef SKIP_FORMAT_BUCK_CHECKS $(MAKE) check-format $(MAKE) check-buck-targets + $(MAKE) check-sources endif # TODO add ldb_tests @@ -1220,6 +1221,9 @@ check-format: check-buck-targets: buckifier/check_buck_targets.sh +check-sources: + build_tools/check-sources.sh + package: bash build_tools/make_package.sh $(SHARED_MAJOR).$(SHARED_MINOR) @@ -1897,7 +1901,7 @@ clipping_iterator_test: $(OBJ_DIR)/db/compaction/clipping_iterator_test.o $(TEST ribbon_bench: $(OBJ_DIR)/microbench/ribbon_bench.o $(LIBRARY) $(AM_LINK) - + cache_reservation_manager_test: $(OBJ_DIR)/cache/cache_reservation_manager_test.o $(TEST_LIBRARY) $(LIBRARY) $(AM_LINK) #------------------------------------------------- @@ -2364,7 +2368,7 @@ build_subset_tests: $(ROCKSDBTESTS_SUBSET) # Remove the rules for which dependencies should not be generated and see if any are left. #If so, include the dependencies; if not, do not include the dependency files -ROCKS_DEP_RULES=$(filter-out clean format check-format check-buck-targets jclean jtest package analyze tags rocksdbjavastatic% unity.% unity_test, $(MAKECMDGOALS)) +ROCKS_DEP_RULES=$(filter-out clean format check-format check-buck-targets check-sources jclean jtest package analyze tags rocksdbjavastatic% unity.% unity_test, $(MAKECMDGOALS)) ifneq ("$(ROCKS_DEP_RULES)", "") -include $(DEPFILES) endif diff --git a/build_tools/check-sources.sh b/build_tools/check-sources.sh new file mode 100755 index 000000000..7e481ff89 --- /dev/null +++ b/build_tools/check-sources.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. +# +# Check for some simple mistakes that should prevent commit or push + +BAD="" + +git grep 'namespace rocksdb' -- '*.[ch]*' +if [ "$?" != "1" ]; then + echo "^^^^^ Do not hardcode namespace rocksdb. Use ROCKSDB_NAMESPACE" + BAD=1 +fi + +git grep -i 'nocommit' -- ':!build_tools/check-sources.sh' +if [ "$?" != "1" ]; then + echo "^^^^^ Code was not intended to be committed" + BAD=1 +fi + +git grep ' +#include "rocksdb/cache_bench_tool.h" int main(int argc, char** argv) { return ROCKSDB_NAMESPACE::cache_bench_tool(argc, argv); } diff --git a/db_stress_tool/db_stress.cc b/db_stress_tool/db_stress.cc index e03e96787..f62268d4c 100644 --- a/db_stress_tool/db_stress.cc +++ b/db_stress_tool/db_stress.cc @@ -15,7 +15,7 @@ int main() { return 1; } #else -#include +#include "rocksdb/db_stress_tool.h" int main(int argc, char** argv) { return ROCKSDB_NAMESPACE::db_stress_tool(argc, argv); diff --git a/examples/compaction_filter_example.cc b/examples/compaction_filter_example.cc index bc960e016..ed1ada823 100644 --- a/examples/compaction_filter_example.cc +++ b/examples/compaction_filter_example.cc @@ -3,10 +3,10 @@ // COPYING file in the root directory) and Apache 2.0 License // (found in the LICENSE.Apache file in the root directory). -#include -#include -#include -#include +#include "rocksdb/compaction_filter.h" +#include "rocksdb/db.h" +#include "rocksdb/merge_operator.h" +#include "rocksdb/options.h" class MyMerge : public ROCKSDB_NAMESPACE::MergeOperator { public: diff --git a/include/rocksdb/memtablerep.h b/include/rocksdb/memtablerep.h index a2eeba041..3e4a78616 100644 --- a/include/rocksdb/memtablerep.h +++ b/include/rocksdb/memtablerep.h @@ -35,7 +35,6 @@ #pragma once -#include #include #include @@ -43,6 +42,8 @@ #include #include +#include "rocksdb/slice.h" + namespace ROCKSDB_NAMESPACE { class Arena; diff --git a/include/rocksdb/system_clock.h b/include/rocksdb/system_clock.h index e03d195ee..3a52b42e2 100644 --- a/include/rocksdb/system_clock.h +++ b/include/rocksdb/system_clock.h @@ -7,12 +7,13 @@ // found in the LICENSE file. See the AUTHORS file for names of contributors. #pragma once -#include -#include #include #include +#include "rocksdb/rocksdb_namespace.h" +#include "rocksdb/status.h" + #ifdef _WIN32 // Windows API macro interference #undef GetCurrentTime diff --git a/java/rocksjni/event_listener_jnicallback.cc b/java/rocksjni/event_listener_jnicallback.cc index cfb5d8c23..342d938b4 100644 --- a/java/rocksjni/event_listener_jnicallback.cc +++ b/java/rocksjni/event_listener_jnicallback.cc @@ -10,7 +10,7 @@ #include "rocksjni/portal.h" -namespace rocksdb { +namespace ROCKSDB_NAMESPACE { EventListenerJniCallback::EventListenerJniCallback( JNIEnv* env, jobject jevent_listener, const std::set& enabled_event_callbacks) @@ -499,4 +499,4 @@ void EventListenerJniCallback::OnFileOperation(const jmethodID& mid, CleanupCallbackInvocation(env, attached_thread, {&jop_info}); } -} // namespace rocksdb +} // namespace ROCKSDB_NAMESPACE diff --git a/java/rocksjni/event_listener_jnicallback.h b/java/rocksjni/event_listener_jnicallback.h index b1802b341..f4a235a23 100644 --- a/java/rocksjni/event_listener_jnicallback.h +++ b/java/rocksjni/event_listener_jnicallback.h @@ -17,7 +17,7 @@ #include "rocksdb/listener.h" #include "rocksjni/jnicallback.h" -namespace rocksdb { +namespace ROCKSDB_NAMESPACE { enum EnabledEventCallback { ON_FLUSH_COMPLETED = 0x0, @@ -117,6 +117,6 @@ class EventListenerJniCallback : public JniCallback, public EventListener { jmethodID m_on_error_recovery_completed_mid; }; -} // namespace rocksdb +} // namespace ROCKSDB_NAMESPACE #endif // JAVA_ROCKSJNI_EVENT_LISTENER_JNICALLBACK_H_ diff --git a/memory/memkind_kmem_allocator.cc b/memory/memkind_kmem_allocator.cc index 0ce985f7e..0349c34f9 100644 --- a/memory/memkind_kmem_allocator.cc +++ b/memory/memkind_kmem_allocator.cc @@ -8,7 +8,7 @@ #include "memkind_kmem_allocator.h" -namespace rocksdb { +namespace ROCKSDB_NAMESPACE { void* MemkindKmemAllocator::Allocate(size_t size) { void* p = memkind_malloc(MEMKIND_DAX_KMEM, size); @@ -29,5 +29,5 @@ size_t MemkindKmemAllocator::UsableSize(void* p, } #endif // ROCKSDB_MALLOC_USABLE_SIZE -} // namespace rocksdb +} // namespace ROCKSDB_NAMESPACE #endif // MEMKIND diff --git a/memory/memkind_kmem_allocator.h b/memory/memkind_kmem_allocator.h index 8bf3c6c72..32ad603ff 100644 --- a/memory/memkind_kmem_allocator.h +++ b/memory/memkind_kmem_allocator.h @@ -11,7 +11,7 @@ #include #include "rocksdb/memory_allocator.h" -namespace rocksdb { +namespace ROCKSDB_NAMESPACE { class MemkindKmemAllocator : public MemoryAllocator { public: @@ -23,5 +23,5 @@ class MemkindKmemAllocator : public MemoryAllocator { #endif }; -} // namespace rocksdb +} // namespace ROCKSDB_NAMESPACE #endif // MEMKIND diff --git a/memory/memkind_kmem_allocator_test.cc b/memory/memkind_kmem_allocator_test.cc index 4153f0c23..ea6de188b 100644 --- a/memory/memkind_kmem_allocator_test.cc +++ b/memory/memkind_kmem_allocator_test.cc @@ -14,7 +14,7 @@ #include "table/block_based/block_based_table_factory.h" #include "test_util/testharness.h" -namespace rocksdb { +namespace ROCKSDB_NAMESPACE { TEST(MemkindKmemAllocatorTest, Allocate) { MemkindKmemAllocator allocator; void* p; @@ -84,7 +84,7 @@ TEST(MemkindKmemAllocatorTest, DatabaseBlockCache) { ASSERT_OK(s); ASSERT_OK(DestroyDB(dbname, options)); } -} // namespace rocksdb +} // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); diff --git a/table/block_based/index_builder.cc b/table/block_based/index_builder.cc index 87585562d..f1c5d8582 100644 --- a/table/block_based/index_builder.cc +++ b/table/block_based/index_builder.cc @@ -20,9 +20,8 @@ #include "table/block_based/partitioned_filter_block.h" #include "table/format.h" -// Without anonymous namespace here, we fail the warning -Wmissing-prototypes namespace ROCKSDB_NAMESPACE { -// using namespace rocksdb; + // Create a index builder based on its type. IndexBuilder* IndexBuilder::CreateIndexBuilder( BlockBasedTableOptions::IndexType index_type, diff --git a/test_util/testharness.h b/test_util/testharness.h index eb104b326..e5db00dc6 100644 --- a/test_util/testharness.h +++ b/test_util/testharness.h @@ -14,7 +14,7 @@ #else #include #endif -#include +#include "rocksdb/utilities/regex.h" // A "skipped" test has a specific meaning in Facebook infrastructure: the // test is in good shape and should be run, but something about the diff --git a/tools/db_bench.cc b/tools/db_bench.cc index d4fb50c14..f13de83fe 100644 --- a/tools/db_bench.cc +++ b/tools/db_bench.cc @@ -14,7 +14,7 @@ int main() { return 1; } #else -#include +#include "rocksdb/db_bench_tool.h" int main(int argc, char** argv) { return ROCKSDB_NAMESPACE::db_bench_tool(argc, argv); }