Cpp quality review 3f29968f

.clang-tidy

@@ -46,6 +46,43 @@ Checks: >
   -readability-named-parameter,
   -readability-uppercase-literal-suffix,
   -readability-use-concise-preprocessor-directives,
+  -readability-braces-around-statements,
+  -readability-function-cognitive-complexity,
+  -bugprone-branch-clone,
+  -bugprone-misplaced-widening-cast,
+  -bugprone-narrowing-conversions,
+  -bugprone-multi-level-implicit-pointer-conversion,
+  -bugprone-unsafe-functions,
+  -bugprone-unchecked-string-to-number-conversion,
+  -bugprone-sizeof-expression,
+  -bugprone-derived-method-shadowing-base-method,
+  -cppcoreguidelines-non-private-member-variables-in-classes,
+  -cppcoreguidelines-prefer-member-initializer,
+  -cppcoreguidelines-init-variables,
+  -cppcoreguidelines-virtual-class-destructor,
+  -cppcoreguidelines-use-default-member-init,
+  -cppcoreguidelines-noexcept-move-operations,
+  -cppcoreguidelines-narrowing-conversions,
+  -cppcoreguidelines-pro-type-const-cast,
+  -cppcoreguidelines-pro-type-static-cast-downcast,
+  -cppcoreguidelines-pro-bounds-constant-array-index,
+  -cppcoreguidelines-pro-bounds-array-to-pointer-decay,
+  -cppcoreguidelines-pro-bounds-avoid-unchecked-container-access,
+  -cppcoreguidelines-avoid-const-or-ref-data-members,
+  -cppcoreguidelines-macro-to-enum,
+  -cppcoreguidelines-missing-std-forward,
+  -modernize-redundant-void-arg,
+  -modernize-use-designated-initializers,
+  -modernize-use-ranges,
+  -modernize-use-integer-sign-comparison,
+  -modernize-avoid-variadic-functions,
+  -modernize-macro-to-enum,
+  -modernize-loop-convert,
+  -modernize-use-default-member-init,
+  -performance-noexcept-move-constructor,
+  -readability-avoid-const-params-in-decls,
+  -readability-qualified-auto,
+  -readability-make-member-function-const,
 
 # Treat warnings as errors for these critical checks
 WarningsAsErrors: >

Scripts/run-clang-tidy.sh

@@ -36,11 +36,14 @@ CXXFLAGS="-std=c++20 $SYSROOT \
 -isystem$PROJECT_ROOT/Submodules/swift-runtime-headers/stdlib/include \
 -DCOMPILED_WITH_SWIFT -DPURE_BRIDGING_MODE"
 
-# Generate compile_commands.json
+# Generate compile_commands.json for all C++ targets
 {
     echo "["
     sep=""
-    find "$PROJECT_ROOT/Sources/ComputeCxx" -name '*.cpp' -type f | sort | while read -r file; do
+    find "$PROJECT_ROOT/Sources/ComputeCxx" \
+         "$PROJECT_ROOT/Sources/Platform" \
+         "$PROJECT_ROOT/Sources/Utilities" \
+         -name '*.cpp' -type f 2>/dev/null | sort | while read -r file; do
         printf '%s\n{"directory":"%s","file":"%s","command":"clang++ %s -c %s"}' \
             "$sep" "$PROJECT_ROOT" "$file" "$CXXFLAGS" "$file"
         sep=","
@@ -51,5 +54,5 @@ CXXFLAGS="-std=c++20 $SYSROOT \
 exec python3 "$SCRIPT_DIR/run-clang-tidy.py" \
     -p="$BUILD_DIR" \
     -config-file="$PROJECT_ROOT/.clang-tidy" \
-    -header-filter="$PROJECT_ROOT/Sources/ComputeCxx/.*\.h$" \
+    -header-filter="$PROJECT_ROOT/Sources/(ComputeCxx|Platform|Utilities)/.*\.h$" \
     "$@"

Sources/ComputeCxx/Comparison/Builder.h

@@ -149,13 +149,13 @@ class Builder : public swift::metadata_visitor {
 
     void revert(const RevertItemsInfo &info);
 
-    virtual bool visit_element(const swift::metadata &type, const swift::metadata::ref_kind kind, size_t element_offset,
+    bool visit_element(const swift::metadata &type, const swift::metadata::ref_kind kind, size_t element_offset,
                                size_t element_size) override;
 
-    virtual bool visit_case(const swift::metadata &type, const swift::field_record &field, uint32_t index) override;
-    virtual bool visit_existential(const swift::existential_type_metadata &type) override;
-    virtual bool visit_function(const swift::function_type_metadata &type) override;
-    virtual bool visit_native_object(const swift::metadata &type) override;
+    bool visit_case(const swift::metadata &type, const swift::field_record &field, uint32_t index) override;
+    bool visit_existential(const swift::existential_type_metadata &type) override;
+    bool visit_function(const swift::function_type_metadata &type) override;
+    bool visit_native_object(const swift::metadata &type) override;
 };
 
 } // namespace LayoutDescriptor

Sources/ComputeCxx/Comparison/LayoutDescriptor.cpp

@@ -117,7 +117,7 @@ ValueLayout TypeDescriptorCache::fetch(const swift::metadata &type, IAGCompariso
     lock();
 
     const void *found = nullptr;
-    ValueLayout layout = (ValueLayout)_table.lookup((void *)key, &found);
+    ValueLayout layout = (ValueLayout)_table.lookup(key, &found);
     if (found) {
         _cache_hit_count += 1;
         unlock();
@@ -165,7 +165,7 @@ ValueLayout TypeDescriptorCache::insert_sync(void *key, const swift::metadata &t
         _sync_total_seconds += time;
     }
 
-    _table.insert((void *)key, layout);
+    _table.insert(key, layout);
     unlock();
     return layout;
 }
@@ -175,7 +175,7 @@ ValueLayout TypeDescriptorCache::insert_sync(void *key, const swift::metadata &t
 void TypeDescriptorCache::insert_async(void *key, const swift::metadata &type, IAGComparisonMode comparison_mode,
                                        LayoutDescriptor::HeapMode heap_mode, uint32_t priority) {
     lock();
-    _table.insert((void *)key, nullptr);
+    _table.insert(key, nullptr);
 
     _async_queue.push_back({
         &type,
@@ -1225,7 +1225,7 @@ bool Builder::visit_case(const swift::metadata &type, const swift::field_record
 bool Builder::visit_existential(const swift::existential_type_metadata &type) {
     if (_current_comparison_mode == 0 ||
         (_current_comparison_mode == 1 && type.representation() == ::swift::ExistentialTypeRepresentation::Class)) {
-        return false;
+        return false; // NOLINT(readability-simplify-boolean-expr) -- side effects follow
     }
 
     get_items().push_back(ExistentialItem(RangeItem(_current_offset, type.vw_size()), &type));
@@ -1244,7 +1244,7 @@ bool Builder::visit_function(const swift::function_type_metadata &type) {
 
 bool Builder::visit_native_object(const swift::metadata &type) {
     if (_heap_mode != HeapMode::Locals) {
-        return false;
+        return false; // NOLINT(readability-simplify-boolean-expr) -- side effects follow
     }
 
     get_items().push_back(HeapRefItem(RangeItem(_current_offset, sizeof(void *)), false));

Sources/ComputeCxx/Data/Table.cpp

@@ -24,6 +24,7 @@ static void *IAGGraphVMRegionBaseAddress;
 namespace IAG {
 namespace data {
 
+// NOLINTNEXTLINE(bugprone-throwing-static-initialization) -- storage only, constructed later via placement new
 table _shared_table_bytes;
 
 table &table::ensure_shared() {

Sources/ComputeCxx/Data/Zone.cpp

@@ -178,7 +178,7 @@ void *zone::alloc_persistent(size_t size) {
         return nullptr;
     }
 
-    auto buffer = table::shared().alloc_persistent(size);
+    auto buffer = IAG::data::table::alloc_persistent(size);
     _malloc_buffers.push_back(std::move(buffer));
 
     return _malloc_buffers.back().get();

Sources/ComputeCxx/Debug/Connection.cpp

@@ -119,7 +119,7 @@ void DebugServer::Connection::handler(void *context) {
     void *request_bytes = CFDataGetMutableBytePtr(request_data);
 
     if (blocking_read(connection->_socket, request_bytes, length)) {
-        CFDataRef response_data = connection->_server->receive(connection, header, request_data);
+        CFDataRef response_data = IAG::DebugServer::receive(connection, header, request_data);
         if (response_data) {
             CFIndex response_length = CFDataGetLength(response_data);
             if (response_length >> 32 == 0) {

Sources/ComputeCxx/Debug/DebugServer.cpp

@@ -9,6 +9,8 @@
 #include <sys/socket.h>
 #include <xlocale.h>
 
+#include <algorithm>
+
 #include "Log/Log.h"
 
 namespace IAG {
@@ -19,6 +21,7 @@ DebugServer *DebugServer::_shared_server = nullptr;
 
 DebugServer *_Nullable DebugServer::start(IAGDebugServerOptions options) {
     if (options & IAGDebugServerOptionsEnabled && !_shared_server) {
+        // NOLINTNEXTLINE(readability-simplify-boolean-expr) -- placeholder for future diagnostics check
         if (true /* && os_variant_has_internal_diagnostics() */) {
             _shared_server = new DebugServer(options);
         }
@@ -142,9 +145,7 @@ void DebugServer::run(uint32_t timeout) {
         int nfds = _socket;
         for (auto &connection : _connections) {
             FD_SET(connection->socket(), &writefds);
-            if (connection->socket() > nfds) {
-                nfds = connection->socket();
-            }
+            nfds = std::max(connection->socket(), nfds);
         }
 
         tv.tv_sec = timeout;

Sources/ComputeCxx/Graph/Context.cpp

@@ -1,5 +1,7 @@
 #include "Context.h"
 
+#include <algorithm>
+
 #include "Attribute/AttributeID/AttributeID.h"
 #include "ComputeCxx/IAGUniqueID.h"
 #include "IAGGraph-Private.h"
@@ -23,9 +25,7 @@ Graph::Context::~Context() {
         uint64_t min_deadline = UINT64_MAX;
         _graph->_contexts_by_id.for_each(
             [](const uint64_t context_id, Context *const context, void *min_deadline_ref) {
-                if (context->_deadline < *(uint64_t *)min_deadline_ref) {
-                    *(uint64_t *)min_deadline_ref = context->_deadline;
-                }
+                *(uint64_t *)min_deadline_ref = std::min(context->_deadline, *(uint64_t *)min_deadline_ref);
             },
             &min_deadline);
         _graph->set_deadline(min_deadline);
@@ -61,9 +61,7 @@ void Graph::Context::set_deadline(uint64_t deadline) {
     uint64_t min_deadline = UINT64_MAX;
     _graph->_contexts_by_id.for_each(
         [](const uint64_t context_id, Context *const context, void *min_deadline_ref) {
-            if (context->_deadline < *(uint64_t *)min_deadline_ref) {
-                *(uint64_t *)min_deadline_ref = context->_deadline;
-            }
+            *(uint64_t *)min_deadline_ref = std::min(context->_deadline, *(uint64_t *)min_deadline_ref);
         },
         &min_deadline);
     _graph->set_deadline(min_deadline);

Sources/ComputeCxx/Graph/Graph.cpp

@@ -287,7 +287,8 @@ void Graph::invalidate_subgraphs() {
     }
 
     if (_main_handler == nullptr) {
-        auto iter = _subgraphs_with_cached_nodes.begin(), end = _subgraphs_with_cached_nodes.end();
+        auto iter = _subgraphs_with_cached_nodes.begin();
+        auto end = _subgraphs_with_cached_nodes.end();
         while (iter != end) {
             auto *subgraph = *iter;
             subgraph->set_graph_invalidating_subgraphs(true);
@@ -435,7 +436,7 @@ data::ptr<Node> Graph::add_attribute(Subgraph &subgraph, uint32_t type_id, const
 
     // Initialize value
     if (initial_value != nullptr) {
-        value_set_internal(node_ptr, *node_ptr.get(), initial_value, type.value_metadata());
+        value_set_internal(node_ptr, *node_ptr, initial_value, type.value_metadata());
     } else {
         node_ptr->set_dirty(true);
         node_ptr->set_pending(true);
@@ -593,7 +594,7 @@ bool Graph::remove_removed_output(AttributeID attribute, AttributeID output, boo
         uint32_t index = 0;
         for (auto &input_edge : output_node->input_edges()) {
             if (input_edge.attribute.traverses(attribute, TraversalOptions::SkipMutableReference)) {
-                remove_input_edge(output_node, *output_node.get(), index);
+                remove_input_edge(output_node, *output_node, index);
                 return true;
             }
             index += 1;
@@ -691,14 +692,14 @@ uint32_t Graph::add_input(data::ptr<Node> node, AttributeID input, bool allow_ni
 
 void Graph::remove_input(data::ptr<Node> node, uint32_t index) {
     remove_input_dependencies(AttributeID(node), node->input_edges()[index].attribute);
-    remove_input_edge(node, *node.get(), index);
+    remove_input_edge(node, *node, index);
 }
 
 void Graph::remove_input_edge(data::ptr<Node> node_ptr, Node &node, uint32_t index) {
     foreach_trace([&node_ptr, &index](Trace &trace) { trace.remove_edge(node_ptr, index); });
 
     node.remove_input_edge(index);
-    if (node.input_edges().size() == 0) {
+    if (node.input_edges().empty()) {
         all_inputs_removed(node_ptr);
     }
     reset_update(node_ptr);
@@ -1188,7 +1189,7 @@ Graph::UpdateStatus Graph::update_attribute(data::ptr<Node> node, IAGGraphUpdate
         [&current_update, &node, &options](Trace &trace) { trace.begin_update(current_update, node, options); });
 
     UpdateStatus status = UpdateStatus::Changed;
-    if (current_update.push(node, *node.get(), false, !(options & IAGGraphUpdateOptionsInTransaction))) {
+    if (current_update.push(node, *node, false, !(options & IAGGraphUpdateOptionsInTransaction))) {
         status = current_update.update();
         if (status == UpdateStatus::NeedsCallMainHandler) {
             std::pair<UpdateStack *, UpdateStatus> context = {&current_update, UpdateStatus::NeedsCallMainHandler};
@@ -1502,7 +1503,7 @@ void *Graph::input_value_ref(data::ptr<IAG::Node> node, AttributeID input, uint3
                              IAGChangedValueFlags *_Nonnull flags_out) {
     auto comparator = InputEdge::Comparator(
         input, IAGInputOptionsUnprefetched | IAGInputOptionsSyncMainRef | IAGInputOptionsAlwaysEnabled, input_options);
-    uint32_t index = index_of_input(*node.get(), comparator);
+    uint32_t index = index_of_input(*node, comparator);
 
     if (index < UINT32_MAX) {
         IAG::OffsetAttributeID resolved_input =
@@ -1535,7 +1536,7 @@ void *Graph::input_value_ref_slow(data::ptr<IAG::Node> node, AttributeID input,
     if (input_options & IAGInputOptionsSyncMainRef) {
         auto comparator = InputEdge::Comparator(input, IAGInputOptionsUnprefetched | IAGInputOptionsAlwaysEnabled,
                                                 input_options & IAGInputOptionsUnprefetched);
-        index = index_of_input(*node.get(), comparator);
+        index = index_of_input(*node, comparator);
     }
 
     if (index == UINT32_MAX) {
@@ -1610,7 +1611,7 @@ bool Graph::value_set(data::ptr<Node> node, const swift::metadata &value_type, c
         precondition_failure("setting value during update: %u", node);
     }
 
-    bool changed = value_set_internal(node, *node.get(), value, value_type);
+    bool changed = value_set_internal(node, *node, value, value_type);
     if (changed) {
         propagate_dirty(AttributeID(node));
     }
@@ -1873,7 +1874,7 @@ void Graph::input_value_add(data::ptr<Node> node, AttributeID input, IAGInputOpt
 
     auto comparator = InputEdge::Comparator(input, options & IAGInputOptionsUnprefetched,
                                             IAGInputOptionsUnprefetched | IAGInputOptionsAlwaysEnabled);
-    auto index = index_of_input(*node.get(), comparator);
+    auto index = index_of_input(*node, comparator);
     if (index == UINT32_MAX) {
         index = add_input(node, input, false, options & IAGInputOptionsUnprefetched);
     }

Sources/ComputeCxx/Graph/IAGGraph.cpp

@@ -405,8 +405,8 @@ void IAGGraphSetIndirectDependency(IAGAttribute attribute, IAGAttribute dependen
         IAG::precondition_failure("invalid indirect attribute: %u", attribute);
     }
 
-    return attribute_id.subgraph()->graph()->indirect_attribute_set_dependency(indirect_node,
-                                                                               IAG::AttributeID(dependency));
+    attribute_id.subgraph()->graph()->indirect_attribute_set_dependency(indirect_node,
+                                                                        IAG::AttributeID(dependency));
 }
 
 #pragma mark - Search
@@ -698,7 +698,7 @@ void *IAGGraphReadCachedAttribute(size_t hash, IAGTypeID type, const void *body,
         read_cached_attribute(hash, *metadata, body, *value_metadata, options, owner_id, &flags,
                               IAG::ClosureFunctionCI<uint32_t, IAGUnownedGraphContextRef>(closure, closure_context));
     if (changed_out) {
-        *changed_out = flags & IAGChangedValueFlagsChanged ? true : false;
+        *changed_out = (flags & IAGChangedValueFlagsChanged) != 0;
     }
     return value;
 }
@@ -714,7 +714,7 @@ void *IAGGraphReadCachedAttributeIfExists(size_t hash, IAGTypeID type, const voi
     IAGChangedValueFlags flags = 0;
     void *value = read_cached_attribute(hash, *metadata, body, *value_metadata, options, owner_id, &flags, nullptr);
     if (changed_out) {
-        *changed_out = flags & IAGChangedValueFlagsChanged ? true : false;
+        *changed_out = (flags & IAGChangedValueFlagsChanged) != 0;
     }
     return value;
 }

Sources/ComputeCxx/Graph/UpdateStack.cpp

@@ -20,7 +20,7 @@ Graph::UpdateStack::UpdateStack(Graph *graph, IAGGraphUpdateOptions options)
 
     graph->_current_update_thread = _thread;
 
-    if (graph->_deferring_subgraph_invalidation == false) {
+    if (!graph->_deferring_subgraph_invalidation) {
         graph->_deferring_subgraph_invalidation = true;
         _options = IAGGraphUpdateOptions(_options | IAGGraphUpdateOptionsEndDeferringSubgraphInvalidationOnExit);
     }
@@ -213,7 +213,7 @@ Graph::UpdateStatus Graph::UpdateStack::update() {
                     if (AttributeID dependency = input_attribute.get_indirect_node()->to_mutable().dependency()) {
                         if (!dependency.get_node()->is_value_initialized() || dependency.get_node()->is_dirty()) {
                             frame.num_pushed_inputs = input_index;
-                            push(dependency.get_node(), *dependency.get_node().get(), false, false);
+                            push(dependency.get_node(), *dependency.get_node(), false, false);
                             // go to top regardless
                             return update();
                         }
@@ -231,7 +231,7 @@ Graph::UpdateStatus Graph::UpdateStack::update() {
 
                     if (!(input_edge.options & IAGInputOptionsUnprefetched) && input_attribute.subgraph()->is_valid()) {
                         frame.num_pushed_inputs = input_index + 1;
-                        if (push(input_node, *input_node.get(), true, true)) {
+                        if (push(input_node, *input_node, true, true)) {
                             // go to top
                             return update();
                         }

Sources/ComputeCxx/Subgraph/Subgraph.cpp

@@ -105,10 +105,7 @@ void Subgraph::remove_observer(IAGUniqueID observer_id) {
     if (auto *observers_ptr = _observers.get()) {
         auto *observers = *observers_ptr;
         auto *iter = std::remove_if(observers->begin(), observers->end(), [&observer_id](auto observer) -> bool {
-            if (observer.observer_id == observer_id) {
-                return true;
-            }
-            return false;
+            return observer.observer_id == observer_id;
         });
         observers->erase(iter, observers->end());
     }

Sources/ComputeCxx/Swift/IAGTuple.cpp

@@ -197,15 +197,15 @@ void IAGTupleWithBuffer(IAGTupleType tuple_type, size_t count,
     auto buffer_size = metadata->vw_stride() * count;
     if (buffer_size <= 0x1000) {
         void *buffer = (unsigned char *)alloca(buffer_size);
-        bzero((void *)buffer, buffer_size);
+        bzero(buffer, buffer_size);
         IAGUnsafeMutableTuple tuple = {tuple_type, buffer};
         function(tuple, context);
     } else {
         void *buffer = malloc(buffer_size);
         if (buffer == nullptr) {
             IAG::precondition_failure("memory allocation failure");
         }
-        bzero((void *)buffer, buffer_size);
+        bzero(buffer, buffer_size);
         IAGUnsafeMutableTuple tuple = {tuple_type, buffer};
         function(tuple, context);
         free(buffer);