- Switch some CI jobs to clang 15

- Fix clang-tidy warnings from Clang 15

.clang-tidy

@@ -16,6 +16,9 @@ Checks: |
   -cert-err60-cpp,
   -bugprone-macro-parentheses,
   -bugprone-exception-escape,
+  -bugprone-easily-swappable-parameters,
+  -bugprone-implicit-widening-of-multiplication-result,
+  -bugprone-narrowing-conversions,
   -cppcoreguidelines-owning-memory,
   -cppcoreguidelines-no-malloc,
   -cppcoreguidelines-pro-type-const-cast,
@@ -30,6 +33,7 @@ Checks: |
   -cppcoreguidelines-macro-usage,
   -cppcoreguidelines-non-private-member-variables-in-classes,
   -cppcoreguidelines-avoid-non-const-global-variables,
+  -cppcoreguidelines-narrowing-conversions,
   -clang-analyzer-optin.cplusplus.VirtualCall,
   -clang-analyzer-cplusplus.NewDeleteLeaks,
   -misc-macro-parentheses,

.github/workflows/main.yaml

@@ -180,9 +180,9 @@ jobs:
           - name: Local dependencies
             os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 libomp5-15 libomp-15-dev
             build_type: RelWithDebInfo
             extra_cmake_flags: -DDEPENDENCY_CONFIG=../cmake-utils/DependenciesFromLocalSystem.cmake
             extra_cxxflags: ""
@@ -193,9 +193,9 @@ jobs:
           - name: Local dependencies
             os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 libomp5-15 libomp-15-dev
             build_type: RelWithDebInfo
             extra_cmake_flags: -DDEPENDENCY_CONFIG=../cmake-utils/DependenciesFromLocalSystem.cmake
             extra_cxxflags: ""
@@ -218,9 +218,9 @@ jobs:
           - name: Werror clang
             os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 libomp5-15 libomp-15-dev
             build_type: RelWithDebInfo
             extra_cmake_flags: -DUSE_WERROR=on
             extra_cxxflags: ""
@@ -230,9 +230,9 @@ jobs:
           - name: No compatibility
             os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 libomp5-15 libomp-15-dev
             build_type: RelWithDebInfo
             extra_cmake_flags: ""
             extra_cxxflags: "-DCRYFS_NO_COMPATIBILITY"
@@ -241,12 +241,11 @@ jobs:
             run_build: true
             run_tests: true
           - name: ASAN
-            # TODO Update to ubuntu-22.04
-            os: ubuntu-20.04
+            os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 libomp5-15 libomp-15-dev
             build_type: Debug
             # OpenMP crashes under asan. Disable OpenMP.
             # TODO is it enough to replace this with omp_num_threads: 1 ?
@@ -257,12 +256,11 @@ jobs:
             run_build: true
             run_tests: true
           - name: UBSAN
-            # TODO Update to ubuntu-22.04
-            os: ubuntu-20.04
+            os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 libomp5-15 libomp-15-dev
             build_type: Debug
             # OpenMP crashes under ubsan. Disable OpenMP.
             # TODO is it enough to replace this with omp_num_threads: 1 ?
@@ -273,12 +271,11 @@ jobs:
             run_build: true
             run_tests: true
           - name: TSAN
-            # TODO Update to ubuntu-22.04
-            os: ubuntu-20.04
+            os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 libomp5-15 libomp-15-dev
             build_type: Debug
             extra_cmake_flags: ""
             extra_cxxflags: "-O2 -fsanitize=thread -fno-omit-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-common"
@@ -290,9 +287,9 @@ jobs:
           - name: clang-tidy
             os: ubuntu-22.04
             compiler:
-              cxx: clang++-11
-              cc: clang-11
-              apt_package: clang-11 clang-tidy-11 libomp5-11 libomp-11-dev
+              cxx: clang++-15
+              cc: clang-15
+              apt_package: clang-15 clang-tidy-15 libomp5-15 libomp-15-dev
             build_type: RelWithDebInfo
             extra_cmake_flags: ""
             extra_cxxflags: ""

run-clang-tidy.sh

@@ -8,9 +8,9 @@
 
 set -e
 
-CXX=clang++-11
-CC=clang-11
-SCRIPT=run-clang-tidy-11.py
+CXX=clang++-15
+CC=clang-15
+SCRIPT=run-clang-tidy-15.py
 
 export NUMCORES=`nproc` && if [ ! -n "$NUMCORES" ]; then export NUMCORES=`sysctl -n hw.ncpu`; fi
 echo Using ${NUMCORES} cores

src/blobstore/implementations/onblocks/BlobOnBlocks.h

@@ -21,7 +21,7 @@ class DataTreeRef;
 class BlobOnBlocks final: public Blob {
 public:
   BlobOnBlocks(cpputils::unique_ref<parallelaccessdatatreestore::DataTreeRef> datatree);
-  ~BlobOnBlocks();
+  ~BlobOnBlocks() override;
 
   const blockstore::BlockId &blockId() const override;
 

src/blobstore/implementations/onblocks/BlobStoreOnBlocks.h

@@ -17,7 +17,7 @@ class ParallelAccessDataTreeStore;
 class BlobStoreOnBlocks final: public BlobStore {
 public:
   BlobStoreOnBlocks(cpputils::unique_ref<blockstore::BlockStore> blockStore, uint64_t physicalBlocksizeBytes);
-  ~BlobStoreOnBlocks();
+  ~BlobStoreOnBlocks() override;
 
   cpputils::unique_ref<Blob> create() override;
   boost::optional<cpputils::unique_ref<Blob>> load(const blockstore::BlockId &blockId) override;

src/blobstore/implementations/onblocks/datanodestore/DataInnerNode.h

@@ -17,7 +17,7 @@ class DataInnerNode final: public DataNode {
   using ChildEntry = DataInnerNode_ChildEntry;
 
   DataInnerNode(DataNodeView block);
-  ~DataInnerNode();
+  ~DataInnerNode() override;
 
   uint32_t maxStoreableChildren() const;
 

src/blobstore/implementations/onblocks/datanodestore/DataLeafNode.cpp

@@ -26,13 +26,13 @@ DataLeafNode::~DataLeafNode() {
 
 unique_ref<DataLeafNode> DataLeafNode::CreateNewNode(BlockStore *blockStore, const DataNodeLayout &layout, Data data) {
   ASSERT(data.size() <= layout.maxBytesPerLeaf(), "Data passed in is too large for one leaf.");
-  uint32_t size = data.size();
+  const uint32_t size = data.size();
   return make_unique_ref<DataLeafNode>(DataNodeView::create(blockStore, layout, DataNode::FORMAT_VERSION_HEADER, 0, size, std::move(data)));
 }
 
 unique_ref<DataLeafNode> DataLeafNode::OverwriteNode(BlockStore *blockStore, const DataNodeLayout &layout, const BlockId &blockId, Data data) {
   ASSERT(data.size() == layout.maxBytesPerLeaf(), "Data passed in is too large for one leaf.");
-  uint32_t size = data.size();
+  const uint32_t size = data.size();
   return make_unique_ref<DataLeafNode>(DataNodeView::overwrite(blockStore, layout, DataNode::FORMAT_VERSION_HEADER, 0, size, blockId, std::move(data)));
 }
 
@@ -52,7 +52,7 @@ uint32_t DataLeafNode::numBytes() const {
 
 void DataLeafNode::resize(uint32_t new_size) {
   ASSERT(new_size <= maxStoreableBytes(), "Trying to resize to a size larger than the maximal size");
-  uint32_t old_size = node().Size();
+  const uint32_t old_size = node().Size();
   if (new_size < old_size) {
     fillDataWithZeroesFromTo(new_size, old_size);
   }

src/blobstore/implementations/onblocks/datanodestore/DataLeafNode.h

@@ -15,7 +15,7 @@ class DataLeafNode final: public DataNode {
   static cpputils::unique_ref<DataLeafNode> OverwriteNode(blockstore::BlockStore *blockStore, const DataNodeLayout &layout, const blockstore::BlockId &blockId, cpputils::Data data);
 
   DataLeafNode(DataNodeView block);
-  ~DataLeafNode();
+  ~DataLeafNode() override;
 
   //Returning uint64_t, because calculations handling this probably need to be done in 64bit to support >4GB blobs.
   uint64_t maxStoreableBytes() const;

src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.cpp

@@ -80,7 +80,7 @@ unique_ref<DataNode> DataNodeStore::overwriteNodeWith(unique_ref<DataNode> targe
 }
 
 void DataNodeStore::remove(unique_ref<DataNode> node) {
-  BlockId blockId = node->blockId();
+  const BlockId blockId = node->blockId();
   cpputils::destruct(std::move(node));
   remove(blockId);
 }

src/blobstore/implementations/onblocks/datanodestore/DataNodeView.h

@@ -67,7 +67,7 @@ class DataNodeView final {
 
   static DataNodeView create(blockstore::BlockStore *blockStore, const DataNodeLayout &layout, uint16_t formatVersion, uint8_t depth, uint32_t size, cpputils::Data data) {
     ASSERT(data.size() <= layout.datasizeBytes(), "Data is too large for node");
-    cpputils::Data serialized = serialize_(layout, formatVersion, depth, size, std::move(data));
+    const cpputils::Data serialized = serialize_(layout, formatVersion, depth, size, std::move(data));
     ASSERT(serialized.size() == layout.blocksizeBytes(), "Wrong block size");
     auto block = blockStore->create(serialized);
     return DataNodeView(std::move(block));

src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp

@@ -51,15 +51,15 @@ const BlockId &DataTree::blockId() const {
 void DataTree::flush() const {
   // By grabbing a lock, we ensure that all modifying functions don't run currently and are therefore flushed.
   // It's only a shared lock, because this doesn't modify the tree structure.
-  shared_lock<shared_mutex> lock(_treeStructureMutex);
+  const shared_lock<shared_mutex> lock(_treeStructureMutex);
   // We also have to flush the root node
   _rootNode->flush();
 }
 
 unique_ref<DataNode> DataTree::releaseRootNode() {
   // Lock also ensures that the root node is currently set (traversing unsets it temporarily)
   // It's a unique lock because this "modifies" tree structure by changing _rootNode.
-  unique_lock<shared_mutex> lock(_treeStructureMutex);
+  const unique_lock<shared_mutex> lock(_treeStructureMutex);
   return std::move(_rootNode);
 }
 
@@ -74,13 +74,13 @@ uint32_t DataTree::numNodes() const {
 }
 
 uint32_t DataTree::numLeaves() const {
-  shared_lock<shared_mutex> lock(_treeStructureMutex);
+  const shared_lock<shared_mutex> lock(_treeStructureMutex);
 
   return _getOrComputeSizeCache().numLeaves;
 }
 
 uint64_t DataTree::numBytes() const {
-  shared_lock<shared_mutex> lock(_treeStructureMutex);
+  const shared_lock<shared_mutex> lock(_treeStructureMutex);
   return _numBytes();
 }
 
@@ -107,11 +107,11 @@ DataTree::SizeCache DataTree::_computeSizeCache(const DataNode &node) const {
   }
 
   const DataInnerNode &inner = dynamic_cast<const DataInnerNode&>(node);
-  uint32_t numLeavesInLeftChildren = static_cast<uint32_t>(inner.numChildren()-1) * _leavesPerFullChild(inner);
-  uint64_t numBytesInLeftChildren = numLeavesInLeftChildren * _nodeStore->layout().maxBytesPerLeaf();
+  const uint32_t numLeavesInLeftChildren = static_cast<uint32_t>(inner.numChildren()-1) * _leavesPerFullChild(inner);
+  const uint64_t numBytesInLeftChildren = numLeavesInLeftChildren * _nodeStore->layout().maxBytesPerLeaf();
   auto lastChild = _nodeStore->load(inner.readLastChild().blockId());
   ASSERT(lastChild != none, "Couldn't load last child");
-  SizeCache sizeInRightChild = _computeSizeCache(**lastChild);
+  const SizeCache sizeInRightChild = _computeSizeCache(**lastChild);
 
   return SizeCache {
     numLeavesInLeftChildren + sizeInRightChild.numLeaves,
@@ -136,16 +136,16 @@ void DataTree::_traverseLeavesByByteIndices(uint64_t beginByte, uint64_t sizeByt
     return;
   }
 
-  uint64_t endByte = beginByte + sizeBytes;
-  uint64_t _maxBytesPerLeaf = maxBytesPerLeaf();
-  uint32_t firstLeaf = beginByte / _maxBytesPerLeaf;
-  uint32_t endLeaf = utils::ceilDivision(endByte, _maxBytesPerLeaf);
+  const uint64_t endByte = beginByte + sizeBytes;
+  const uint64_t _maxBytesPerLeaf = maxBytesPerLeaf();
+  const uint32_t firstLeaf = beginByte / _maxBytesPerLeaf;
+  const uint32_t endLeaf = utils::ceilDivision(endByte, _maxBytesPerLeaf);
   bool blobIsGrowingFromThisTraversal = false;
   auto _onExistingLeaf = [&onExistingLeaf, beginByte, endByte, endLeaf, _maxBytesPerLeaf, &blobIsGrowingFromThisTraversal] (uint32_t leafIndex, bool isRightBorderLeaf, LeafHandle leafHandle) {
-    uint64_t indexOfFirstLeafByte = leafIndex * _maxBytesPerLeaf;
+    const uint64_t indexOfFirstLeafByte = leafIndex * _maxBytesPerLeaf;
     ASSERT(endByte > indexOfFirstLeafByte, "Traversal went too far right");
-    uint32_t dataBegin = utils::maxZeroSubtraction(beginByte, indexOfFirstLeafByte);
-    uint32_t dataEnd = std::min(_maxBytesPerLeaf, endByte - indexOfFirstLeafByte);
+    const uint32_t dataBegin = utils::maxZeroSubtraction(beginByte, indexOfFirstLeafByte);
+    const uint32_t dataEnd = std::min(_maxBytesPerLeaf, endByte - indexOfFirstLeafByte);
     // If we are traversing exactly until the last leaf, then the last leaf wasn't resized by the traversal and might have a wrong size. We have to fix it.
     if (isRightBorderLeaf) {
       ASSERT(leafIndex == endLeaf-1, "If we traversed further right, this wouldn't be the right border leaf.");
@@ -160,10 +160,10 @@ void DataTree::_traverseLeavesByByteIndices(uint64_t beginByte, uint64_t sizeByt
   auto _onCreateLeaf = [&onCreateLeaf, _maxBytesPerLeaf, beginByte, firstLeaf, endByte, endLeaf, &blobIsGrowingFromThisTraversal, readOnlyTraversal] (uint32_t leafIndex) -> Data {
     ASSERT(!readOnlyTraversal, "Cannot create leaves in a read-only traversal");
     blobIsGrowingFromThisTraversal = true;
-    uint64_t indexOfFirstLeafByte = leafIndex * _maxBytesPerLeaf;
+    const uint64_t indexOfFirstLeafByte = leafIndex * _maxBytesPerLeaf;
     ASSERT(endByte > indexOfFirstLeafByte, "Traversal went too far right");
-    uint32_t dataBegin = utils::maxZeroSubtraction(beginByte, indexOfFirstLeafByte);
-    uint32_t dataEnd = std::min(_maxBytesPerLeaf, endByte - indexOfFirstLeafByte);
+    const uint32_t dataBegin = utils::maxZeroSubtraction(beginByte, indexOfFirstLeafByte);
+    const uint32_t dataEnd = std::min(_maxBytesPerLeaf, endByte - indexOfFirstLeafByte);
     ASSERT(leafIndex == firstLeaf || dataBegin == 0, "Only the leftmost leaf can have a gap on the left.");
     ASSERT(leafIndex == endLeaf-1 || dataEnd == _maxBytesPerLeaf, "Only the rightmost leaf can have a gap on the right");
     Data data = onCreateLeaf(indexOfFirstLeafByte + dataBegin, dataEnd-dataBegin);
@@ -195,11 +195,11 @@ uint32_t DataTree::_leavesPerFullChild(const DataInnerNode &root) const {
 }
 
 void DataTree::resizeNumBytes(uint64_t newNumBytes) {
-  std::unique_lock<shared_mutex> lock(_treeStructureMutex);
+  const std::unique_lock<shared_mutex> lock(_treeStructureMutex);
 
-  uint32_t newNumLeaves = std::max(UINT64_C(1), utils::ceilDivision(newNumBytes, _nodeStore->layout().maxBytesPerLeaf()));
-  uint32_t newLastLeafSize = newNumBytes - (newNumLeaves-1) * _nodeStore->layout().maxBytesPerLeaf();
-  uint32_t maxChildrenPerInnerNode = _nodeStore->layout().maxChildrenPerInnerNode();
+  const uint32_t newNumLeaves = std::max(UINT64_C(1), utils::ceilDivision(newNumBytes, _nodeStore->layout().maxBytesPerLeaf()));
+  const uint32_t newLastLeafSize = newNumBytes - (newNumLeaves-1) * _nodeStore->layout().maxBytesPerLeaf();
+  const uint32_t maxChildrenPerInnerNode = _nodeStore->layout().maxChildrenPerInnerNode();
   auto onExistingLeaf = [newLastLeafSize] (uint32_t /*index*/, bool /*isRightBorderLeaf*/, LeafHandle leafHandle) {
       auto leaf = leafHandle.node();
       // This is only called, if the new last leaf was already existing
@@ -214,10 +214,10 @@ void DataTree::resizeNumBytes(uint64_t newNumBytes) {
   auto onBacktrackFromSubtree = [this, newNumLeaves, maxChildrenPerInnerNode] (DataInnerNode* node) {
       // This is only called for the right border nodes of the new tree.
       // When growing size, the following is a no-op. When shrinking, we're deleting the children that aren't needed anymore.
-      uint32_t maxLeavesPerChild = utils::intPow(static_cast<uint64_t>(maxChildrenPerInnerNode), (static_cast<uint64_t>(node->depth())-1));
-      uint32_t neededNodesOnChildLevel = utils::ceilDivision(newNumLeaves, maxLeavesPerChild);
-      uint32_t neededSiblings = utils::ceilDivision(neededNodesOnChildLevel, maxChildrenPerInnerNode);
-      uint32_t neededChildrenForRightBorderNode = neededNodesOnChildLevel - (neededSiblings-1) * maxChildrenPerInnerNode;
+      const uint32_t maxLeavesPerChild = utils::intPow(static_cast<uint64_t>(maxChildrenPerInnerNode), (static_cast<uint64_t>(node->depth())-1));
+      const uint32_t neededNodesOnChildLevel = utils::ceilDivision(newNumLeaves, maxLeavesPerChild);
+      const uint32_t neededSiblings = utils::ceilDivision(neededNodesOnChildLevel, maxChildrenPerInnerNode);
+      const uint32_t neededChildrenForRightBorderNode = neededNodesOnChildLevel - (neededSiblings-1) * maxChildrenPerInnerNode;
       ASSERT(neededChildrenForRightBorderNode <= node->numChildren(), "Node has too few children");
       // All children to the right of the new right-border-node are removed including their subtree.
       while(node->numChildren() > neededChildrenForRightBorderNode) {
@@ -238,12 +238,12 @@ uint64_t DataTree::maxBytesPerLeaf() const {
 }
 
 uint8_t DataTree::depth() const {
-  shared_lock<shared_mutex> lock(_treeStructureMutex);
+  const shared_lock<shared_mutex> lock(_treeStructureMutex);
   return _rootNode->depth();
 }
 
 void DataTree::readBytes(void *target, uint64_t offset, uint64_t count) const {
-  shared_lock<shared_mutex> lock(_treeStructureMutex);
+  const shared_lock<shared_mutex> lock(_treeStructureMutex);
 
   const uint64_t _size = _numBytes();
   if(offset > _size || offset + count > _size) {
@@ -256,18 +256,18 @@ void DataTree::readBytes(void *target, uint64_t offset, uint64_t count) const {
 }
 
 Data DataTree::readAllBytes() const {
-  shared_lock<shared_mutex> lock(_treeStructureMutex);
+  const shared_lock<shared_mutex> lock(_treeStructureMutex);
 
   //TODO Querying numBytes can be inefficient. Is this possible without a call to size()?
-  uint64_t count = _numBytes();
+  const uint64_t count = _numBytes();
   Data result(count);
   _doReadBytes(result.data(), 0, count);
 
   return result;
 }
 
 uint64_t DataTree::tryReadBytes(void *target, uint64_t offset, uint64_t count) const {
-  shared_lock<shared_mutex> lock(_treeStructureMutex);
+  const shared_lock<shared_mutex> lock(_treeStructureMutex);
   auto result = _tryReadBytes(target, offset, count);
   return result;
 }
@@ -294,7 +294,7 @@ void DataTree::_doReadBytes(void *target, uint64_t offset, uint64_t count) const
 }
 
 void DataTree::writeBytes(const void *source, uint64_t offset, uint64_t count) {
-  unique_lock<shared_mutex> lock(_treeStructureMutex);
+  const unique_lock<shared_mutex> lock(_treeStructureMutex);
 
   auto onExistingLeaf = [source, offset, count] (uint64_t indexOfFirstLeafByte, LeafHandle leaf, uint32_t leafDataOffset, uint32_t leafDataSize) {
     ASSERT(indexOfFirstLeafByte+leafDataOffset>=offset && indexOfFirstLeafByte-offset+leafDataOffset <= count && indexOfFirstLeafByte-offset+leafDataOffset+leafDataSize <= count, "Reading from source out of bounds");