Merge branch 'feature/clang-tidy' into develop

.circleci/config.yml

@@ -57,9 +57,11 @@ references:
           # They aren't set automatically unfortunately
           sudo ln -s /usr/bin/$CC /usr/bin/clang
           sudo ln -s /usr/bin/$CXX /usr/bin/clang++
+          sudo ln -s /usr/bin/clang-tidy-7 /usr/bin/clang-tidy
+          sudo ln -s /usr/bin/run-clang-tidy-7.py /usr/bin/run-clang-tidy.py
 
           # Need a c++14 compliant STL for clang
-          sudo apt-get install g++-5
+          sudo apt-get install -y g++-5
           sudo apt-get remove g++-4.8 gcc-4.8
         fi
 
@@ -457,6 +459,36 @@ jobs:
       DISABLE_BROKEN_TSAN_TESTS: true
       CMAKE_FLAGS: ""
       RUN_TESTS: true
+  clang_tidy:
+    <<: *container_config
+    steps:
+    - <<: *cache_init
+    - <<: *container_setup_pre
+    - <<: *container_setup
+    - <<: *container_setup_post
+    - <<: *upgrade_boost_pre
+    - <<: *upgrade_boost
+    - <<: *upgrade_boost_post
+    - checkout
+    - run:
+        name: clang-tidy
+        command: |
+                # realpath, jq are needed for run-clang-tidy.sh, g++ is needed for pyyaml
+                sudo apt-get install realpath g++ jq
+                pip install pyyaml
+                mkdir cmake
+                cd cmake
+                if ! ../run-clang-tidy.sh -fix ; then
+                  git diff > /tmp/clang-tidy-fixes
+                  exit 1
+                fi
+    - store_artifacts:
+        path: /tmp/clang-tidy-fixes
+    environment:
+      CC: clang-7
+      CXX: clang++-7
+      BUILD_TOOLSET: clang
+      APT_COMPILER_PACKAGE: "clang-7 clang-tidy-7"
 
 workflows:
   version: 2
@@ -507,3 +539,5 @@ workflows:
         <<: *enable_for_tags
     - thread_sanitizer:
         <<: *enable_for_tags
+    - clang_tidy:
+        <<: *enable_for_tags

.clang-tidy

@@ -12,6 +12,9 @@ Checks: |
   boost-use-to-string,
   -cert-env33-c,
   -cert-err58-cpp,
+  -cert-err60-cpp,
+  -bugprone-macro-parentheses,
+  -bugprone-exception-escape,
   -cppcoreguidelines-owning-memory,
   -cppcoreguidelines-no-malloc,
   -cppcoreguidelines-pro-type-const-cast,
@@ -21,10 +24,11 @@ Checks: |
   -cppcoreguidelines-pro-type-cstyle-cast,
   -cppcoreguidelines-pro-bounds-array-to-pointer-decay,
   -cppcoreguidelines-pro-type-vararg,
+  -cppcoreguidelines-avoid-goto,
+  -clang-analyzer-optin.cplusplus.VirtualCall,
   -misc-macro-parentheses,
   -misc-unused-raii
-#WarningsAsErrors: '*'
-WarningsAsErrors: ''
+WarningsAsErrors: '*'
 HeaderFilterRegex: '/src/|/test/'
 CheckOptions:
   - key:             google-readability-braces-around-statements.ShortStatementLines

cmake-utils/utils.cmake

@@ -61,7 +61,7 @@ function(target_enable_style_warnings TARGET)
     elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang")
         target_compile_options(${TARGET} PRIVATE -Wall -Wextra -Wold-style-cast -Wcast-align -Wno-unused-command-line-argument) # TODO consider -Wpedantic -Wchkp -Wcast-qual -Wctor-dtor-privacy -Wdisabled-optimization -Wformat=2 -Winit-self -Wlogical-op -Wmissing-include-dirs -Wnoexcept -Wold-style-cast -Woverloaded-virtual -Wredundant-decls -Wshadow -Wsign-promo -Wstrict-null-sentinel -Wstrict-overflow=5 -Wundef -Wno-unused -Wno-variadic-macros -Wno-parentheses -fdiagnostics-show-option -Wconversion and others?
     elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
-        target_compile_options(${TARGET} PRIVATE -Wall -Wextra -Wold-style-cast -Wcast-align) # TODO consider -Wpedantic -Wchkp -Wcast-qual -Wctor-dtor-privacy -Wdisabled-optimization -Wformat=2 -Winit-self -Wlogical-op -Wmissing-include-dirs -Wnoexcept -Wold-style-cast -Woverloaded-virtual -Wredundant-decls -Wshadow -Wsign-promo -Wstrict-null-sentinel -Wstrict-overflow=5 -Wundef -Wno-unused -Wno-variadic-macros -Wno-parentheses -fdiagnostics-show-option -Wconversion and others?
+        target_compile_options(${TARGET} PRIVATE -Wall -Wextra -Wold-style-cast -Wcast-align -Wno-maybe-uninitialized) # TODO consider -Wpedantic -Wchkp -Wcast-qual -Wctor-dtor-privacy -Wdisabled-optimization -Wformat=2 -Winit-self -Wlogical-op -Wmissing-include-dirs -Wnoexcept -Wold-style-cast -Woverloaded-virtual -Wredundant-decls -Wshadow -Wsign-promo -Wstrict-null-sentinel -Wstrict-overflow=5 -Wundef -Wno-unused -Wno-variadic-macros -Wno-parentheses -fdiagnostics-show-option -Wconversion and others?
     endif()
 
     if (USE_WERROR)

run-clang-tidy.sh

@@ -8,9 +8,15 @@
 
 set -e
 
-NUMCORES=`nproc`
+export NUMCORES=`nproc` && if [ ! -n "$NUMCORES" ]; then export NUMCORES=`sysctl -n hw.ncpu`; fi
+echo Using ${NUMCORES} cores
 
 # Run cmake in current working directory, but on source that is in the same directory as this script file
 cmake -DBUILD_TESTING=on -DCMAKE_EXPORT_COMPILE_COMMANDS=ON "${0%/*}"
 
+# Filter all third party code from the compilation database
+cat compile_commands.json|jq "map(select(.file | test(\"^$(realpath ${0%/*})/(src|test)/.*$\")))" > compile_commands2.json
+rm compile_commands.json
+mv compile_commands2.json compile_commands.json
+
 run-clang-tidy.py -j${NUMCORES} -quiet -header-filter "$(realpath ${0%/*})/(src|test)/.*" $@

src/cpp-utils/crypto/symmetric/testutils/FakeAuthenticatedCipher.h

@@ -14,7 +14,7 @@ namespace cpputils {
 
     struct FakeKey {
         static FakeKey FromString(const std::string& keyData) {
-          return FakeKey{static_cast<uint64_t>(std::atoi(keyData.c_str()))};
+          return FakeKey{static_cast<uint64_t>(std::strtol(keyData.c_str(), nullptr, 10))};
         }
 
         static constexpr unsigned int BINARY_LENGTH = sizeof(uint64_t);

src/cpp-utils/data/Data.cpp

@@ -47,21 +47,25 @@ Data Data::LoadFromStream(istream &stream, size_t size) {
 Data Data::FromString(const std::string &data, unique_ref<Allocator> allocator) {
   ASSERT(data.size() % 2 == 0, "hex encoded data cannot have odd number of characters");
   Data result(data.size() / 2, std::move(allocator));
-  CryptoPP::StringSource(data, true,
-    new CryptoPP::HexDecoder(
-      new CryptoPP::ArraySink(static_cast<CryptoPP::byte*>(result._data), result.size())
-    )
-  );
+  {
+    CryptoPP::StringSource _1(data, true,
+      new CryptoPP::HexDecoder(
+        new CryptoPP::ArraySink(static_cast<CryptoPP::byte*>(result._data), result.size())
+      )
+    );
+  }
   return result;
 }
 
 std::string Data::ToString() const {
   std::string result;
-  CryptoPP::ArraySource(static_cast<const CryptoPP::byte*>(_data), _size, true,
-    new CryptoPP::HexEncoder(
-        new CryptoPP::StringSink(result)
-    )
-  );
+  {
+    CryptoPP::ArraySource _1(static_cast<const CryptoPP::byte*>(_data), _size, true,
+      new CryptoPP::HexEncoder(
+          new CryptoPP::StringSink(result)
+      )
+    );
+  }
   ASSERT(result.size() == 2 * _size, "Created wrongly sized string");
   return result;
 }

src/cpp-utils/data/FixedSizeData.h

@@ -59,11 +59,13 @@ template<size_t SIZE>
 FixedSizeData<SIZE> FixedSizeData<SIZE>::FromString(const std::string &data) {
   ASSERT(data.size() == STRING_LENGTH, "Wrong string size for parsing FixedSizeData");
   FixedSizeData<SIZE> result;
-  CryptoPP::StringSource(data, true,
-    new CryptoPP::HexDecoder(
-      new CryptoPP::ArraySink(result._data, BINARY_LENGTH)
-    )
-  );
+  {
+    CryptoPP::StringSource _1(data, true,
+      new CryptoPP::HexDecoder(
+        new CryptoPP::ArraySink(result._data, BINARY_LENGTH)
+      )
+    );
+  }
   return result;
 }
 

src/cpp-utils/pointer/cast.h

@@ -15,7 +15,7 @@ inline std::unique_ptr<DST> dynamic_pointer_move(std::unique_ptr<SRC> &source) {
   //TODO Deleter
   DST *casted = dynamic_cast<DST*>(source.get());
   if (casted != nullptr) {
-    source.release();
+    std::ignore = source.release();
   }
   return std::unique_ptr<DST>(casted);
 }

src/cpp-utils/pointer/unique_ref.h

@@ -108,7 +108,7 @@ private:
     explicit unique_ref(std::unique_ptr<T, D> target) noexcept
     : _target(std::move(target)) {}
 
-    void _invariant() const {
+    void _invariant() const noexcept {
         // TODO Test performance impact of this
         ASSERT(_target.get() != nullptr, "Member was moved out to another unique_ref. This instance is invalid.");
     }

src/cpp-utils/system/filetime_nonwindows.cpp

@@ -22,7 +22,7 @@ int set_filetime(const char *filepath, timespec lastAccessTime, timespec lastMod
 }
 
 int get_filetime(const char *filepath, timespec* lastAccessTime, timespec* lastModificationTime) {
-	struct ::stat attrib;
+	struct ::stat attrib{};
 	int retval = ::stat(filepath, &attrib);
 	if (retval != 0) {
 		return errno;

src/cpp-utils/system/time.cpp

@@ -11,7 +11,7 @@ namespace time {
 
 struct timespec now() {
 	auto now = system_clock::now().time_since_epoch();
-	struct timespec spec;
+	struct timespec spec{};
 	spec.tv_sec = duration_cast<seconds>(now).count();
 	spec.tv_nsec = duration_cast<nanoseconds>(now).count() % 1000000000;
 	return spec;

src/cpp-utils/value_type/ValueType.h

@@ -139,7 +139,7 @@ public:
         return *static_cast<ConcreteType*>(this);
     }
 
-    constexpr ConcreteType operator++(int) noexcept(noexcept(++std::declval<ConcreteType>())) {
+    constexpr const ConcreteType operator++(int) noexcept(noexcept(++std::declval<ConcreteType>())) {
         ConcreteType tmp = *static_cast<ConcreteType*>(this);
         ++(*this);
         return tmp;
@@ -150,7 +150,7 @@ public:
         return *static_cast<ConcreteType*>(this);
     }
 
-    constexpr ConcreteType operator--(int) noexcept(noexcept(--std::declval<ConcreteType>())) {
+    constexpr const ConcreteType operator--(int) noexcept(noexcept(--std::declval<ConcreteType>())) {
         ConcreteType tmp = *static_cast<ConcreteType*>(this);
         --(*this);
         return tmp;

src/cryfs-cli/Cli.cpp

@@ -44,8 +44,6 @@ using cpputils::unique_ref;
 using cpputils::SCryptSettings;
 using cpputils::Console;
 using cpputils::HttpClient;
-using cpputils::DontEchoStdinToStdoutRAII;
-using std::cin;
 using std::cout;
 using std::string;
 using std::endl;
@@ -272,7 +270,7 @@ namespace cryfs {
         if (minutes == none) {
             return none;
         }
-        uint64_t millis = std::round(60000 * (*minutes));
+        uint64_t millis = std::llround(60000 * (*minutes));
         return make_unique_ref<CallAfterTimeout>(milliseconds(millis), callback);
     }
 

src/cryfs-cli/program_options/Parser.cpp

@@ -12,10 +12,8 @@ using namespace cryfs::program_options;
 using cryfs::CryConfigConsole;
 using cryfs::CryfsException;
 using cryfs::ErrorCode;
-using std::pair;
 using std::vector;
 using std::cerr;
-using std::cout;
 using std::endl;
 using std::string;
 using boost::optional;

src/cryfs/config/CryCipher.cpp

@@ -8,7 +8,6 @@ using std::vector;
 using std::string;
 using cpputils::unique_ref;
 using cpputils::make_unique_ref;
-using cpputils::FixedSizeData;
 using blockstore::BlockStore2;
 using std::shared_ptr;
 using std::make_shared;

src/cryfs/config/crypto/CryConfigEncryptor.cpp

@@ -5,7 +5,6 @@ using std::string;
 using cpputils::unique_ref;
 using cpputils::make_unique_ref;
 using cpputils::Data;
-using cpputils::FixedSizeData;
 using boost::optional;
 using boost::none;
 using namespace cpputils::logging;

src/fspp/fs_interface/Types.h

@@ -157,15 +157,15 @@ struct num_bytes_t final : cpputils::value_type::QuantityValueType<num_bytes_t,
 };
 
 struct stat_info final {
-    uint32_t nlink;
+    uint32_t nlink{};
     fspp::mode_t mode;
     fspp::uid_t uid;
     fspp::gid_t gid;
     fspp::num_bytes_t size;
-    uint64_t blocks;
-    struct timespec atime;
-    struct timespec mtime;
-    struct timespec ctime;
+    uint64_t blocks{};
+    struct timespec atime{};
+    struct timespec mtime{};
+    struct timespec ctime{};
 };
 
 struct statvfs final {

test/blobstore/implementations/onblocks/datanodestore/DataNodeStoreTest.cpp

@@ -147,4 +147,4 @@ TEST_F(DataNodeStoreTest, PhysicalBlockSize_Inner) {
   auto node = nodeStore->createNewInnerNode(1, {leaf->blockId()});
   auto block = blockStore->load(node->blockId()).value();
   EXPECT_EQ(BLOCKSIZE_BYTES, block->size());
-}
+}

test/blockstore/implementations/encrypted/EncryptedBlockStoreTest_Generic.cpp

@@ -20,7 +20,6 @@ using cpputils::AES256_GCM;
 using cpputils::AES256_CFB;
 using cpputils::FakeAuthenticatedCipher;
 
-using cpputils::Data;
 using cpputils::DataFixture;
 using cpputils::make_unique_ref;
 using cpputils::unique_ref;

test/blockstore/implementations/integrity/KnownBlockVersionsTest.cpp

@@ -231,7 +231,9 @@ TEST_F(KnownBlockVersionsTest, checkAndUpdate_doesntAllowRollbackToOldClientWith
 
 TEST_F(KnownBlockVersionsTest, saveAndLoad_empty) {
     TempFile stateFile(false);
-    KnownBlockVersions(stateFile.path(), myClientId);
+    {
+      KnownBlockVersions _1(stateFile.path(), myClientId);
+    }
 
     EXPECT_TRUE(KnownBlockVersions(stateFile.path(), myClientId).checkAndUpdateVersion(clientId, blockId, 1));
 }

test/cpp-utils/assert/assert_release_test.cpp

@@ -8,7 +8,6 @@
 #endif
 #include "cpp-utils/assert/assert.h"
 
-using testing::MatchesRegex;
 using testing::HasSubstr;
 
 TEST(AssertTest_ReleaseBuild, DoesntThrowIfTrue) {
@@ -32,7 +31,7 @@ TEST(AssertTest_ReleaseBuild, AssertMessage) {
 	  /*EXPECT_THAT(e.what(), MatchesRegex(
 		  R"(Assertion \[2==5\] failed in .*assert_release_test.cpp:27: my message)"
 	  ));*/
-	  EXPECT_TRUE(std::regex_search(e.what(), std::regex(R"(Assertion \[2==5\] failed in .*assert_release_test.cpp:27: my message)")));
+	  EXPECT_TRUE(std::regex_search(e.what(), std::regex(R"(Assertion \[2==5\] failed in .*assert_release_test.cpp:26: my message)")));
   }
 }
 

test/cpp-utils/assert/exit_signal.cpp

@@ -12,20 +12,20 @@ void handle_exit_signal(char* argv[]) {
 		throw std::logic_error(argv[2]);
 	} else if (kind == "nullptr") {
 		int* ptr = nullptr;
-		*ptr = 5;
+		*ptr = 5; // NOLINT
 	} else if (kind == "signal") {
 #if defined(_MSC_VER)
 		DWORD code = std::atoll(argv[2]);
 		::RaiseException(code, EXCEPTION_NONCONTINUABLE, 0, NULL);
 #else
-		int code = std::atoi(argv[2]);
+		int code = std::strtol(argv[2], nullptr, 10);
 		::raise(code);
 #endif
 	}
 }
 
 
-int main(int argc, char* argv[]) {
+int main(int  /*argc*/, char* argv[]) {
 	cpputils::showBacktraceOnCrash();
 #if defined(_MSC_VER)
     // don't show windows error box

test/cpp-utils/data/DataTest.cpp

@@ -197,7 +197,7 @@ TEST_F(DataTest, Inequality_DifferentLastByte) {
 #ifdef __x86_64__
 TEST_F(DataTest, LargesizeSize) {
   //Needs 64bit for representation. This value isn't in the size param list, because the list is also used for read/write checks.
-  uint64_t size = 4.5L*1024*1024*1024;
+  uint64_t size = static_cast<uint64_t>(4.5L*1024*1024*1024);
   Data data(size);
   EXPECT_EQ(size, data.size());
 }
@@ -235,7 +235,7 @@ struct MockAllocator final : public Allocator {
 
 class DataTestWithMockAllocator: public DataTest {
 public:
-    char ptr_target;
+    char ptr_target{};
 
     unique_ref<MockAllocator> allocator = make_unique_ref<MockAllocator>();
     MockAllocator* allocator_ptr = allocator.get();

test/cpp-utils/logging/LoggingLevelTest.cpp

@@ -3,7 +3,6 @@
 
 using namespace cpputils::logging;
 using std::string;
-using testing::MatchesRegex;
 
 class LoggingLevelTest: public LoggingTest {
 public:

test/cpp-utils/logging/LoggingTest.cpp

@@ -8,7 +8,6 @@
 
 using namespace cpputils::logging;
 using std::string;
-using testing::MatchesRegex;
 
 TEST_F(LoggingTest, DefaultLoggerIsStderr) {
     string output = captureStderr([]{

test/cpp-utils/process/exit_status.cpp

@@ -11,6 +11,6 @@ int main(int argc, char* argv[]) {
 
 	std::cout << argv[1];
 
-	int exit_status = std::atoi(argv[2]);
+	int exit_status = std::strtol(argv[2], nullptr, 10);
 	return exit_status;
 }

test/cpp-utils/system/FiletimeTest.cpp

@@ -13,8 +13,8 @@ TEST(FiletimeTest, SetAndGetTime_ReturnsCorrectTime) {
 	int retval = set_filetime(file.path().string().c_str(), accessTime, modificationTime);
 	EXPECT_EQ(0, retval);
 
-	struct timespec readAccessTime;
-	struct timespec readModificationTime;
+	struct timespec readAccessTime{};
+	struct timespec readModificationTime{};
 	retval = get_filetime(file.path().string().c_str(), &readAccessTime, &readModificationTime);
 	EXPECT_EQ(0, retval);
 

test/cryfs/config/CompatibilityTest.cpp

@@ -35,11 +35,13 @@ private:
     Data hexToBinary(const string &hex) {
         ASSERT(hex.size()%2 == 0, "Hex codes need to have two characters per byte");
         Data result(hex.size()/2);
-        CryptoPP::StringSource(hex, true,
-             new CryptoPP::HexDecoder(
-                   new CryptoPP::ArraySink(static_cast<CryptoPP::byte*>(result.data()), result.size())
-             )
-        );
+        {
+            CryptoPP::StringSource _1(hex, true,
+                 new CryptoPP::HexDecoder(
+                       new CryptoPP::ArraySink(static_cast<CryptoPP::byte*>(result.data()), result.size())
+                 )
+            );
+        }
         return result;
     }
 

test/cryfs/config/crypto/CryConfigEncryptorTest.cpp

@@ -9,7 +9,6 @@ using cpputils::make_unique_ref;
 using cpputils::DataFixture;
 using cpputils::Data;
 using cpputils::EncryptionKey;
-using cpputils::FixedSizeData;
 using cpputils::AES128_CFB;
 using cpputils::AES256_GCM;
 using cpputils::Twofish256_GCM;

test/fspp/fuse/read/testutils/FuseReadTest.h

@@ -10,7 +10,7 @@ public:
   const char *FILENAME = "/myfile";
 
   struct ReadError {
-    int error;
+    int error{};
     fspp::num_bytes_t read_bytes;
   };
 

test/fspp/fuse/statfs/FuseStatfsErrorTest.cpp

@@ -2,7 +2,6 @@
 
 #include "fspp/fs_interface/FuseErrnoException.h"
 
-using ::testing::StrEq;
 using ::testing::_;
 using ::testing::Throw;
 using ::testing::Return;

test/fspp/fuse/statfs/testutils/FuseStatfsTest.cpp

@@ -1,7 +1,6 @@
 #include "FuseStatfsTest.h"
 
 using std::function;
-using ::testing::StrEq;
 using ::testing::_;
 using ::testing::Invoke;
 

test/fspp/fuse/write/testutils/FuseWriteTest.h

@@ -10,7 +10,7 @@ public:
   const char *FILENAME = "/myfile";
 
   struct WriteError {
-    int error;
+    int error{};
     fspp::num_bytes_t written_bytes;
   };