Rework type level metrics calculations#802
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR reworks how C++ type-level dependency metrics are calculated by renaming the old metrics table, moving dependency collection into AST parsing, and simplifying both efferent and afferent metrics to single SQL DISTINCT COUNT queries, plus adding missing tests.
- Rename
CppTypeDependencyMetricstoCppTypeDependencyand update ODB views - Persist type dependencies during AST traversal in
ClangASTVisitorand adjust parser to use new views - Add parameterized tests for efferent coupling and update afferent coupling tests' namespace qualifiers
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugins/cpp_metrics/test/src/cppmetricsparsertest.cpp | Add new efferent coupling tests and qualify afferent coupling test entries with the new namespace |
| plugins/cpp_metrics/test/sources/parser/efferentcoupling.cpp | Define test classes under CC_CPP_EFFERENT_COUPLING_METRICS_TEST namespace |
| plugins/cpp_metrics/test/sources/parser/afferentcoupling.cpp | Wrap afferent coupling test sources in CC_CPP_AFFERENT_COUPLING_METRICS_TEST namespace |
| plugins/cpp_metrics/test/sources/parser/CMakeLists.txt | Include efferentcoupling.cpp in the test project |
| plugins/cpp_metrics/parser/src/cppmetricsparser.cpp | Replace manual dependency counting with SQL DISTINCT COUNT queries on CppTypeDependency views |
| plugins/cpp/parser/src/clangastvisitor.h | Collect and persist CppTypeDependency records during AST traversal |
| plugins/cpp/model/include/model/cpptypedependency.h | Introduce CppTypeDependency struct, ODB views for counting, and remove old metrics header |
| plugins/cpp/model/include/model/cppfunction.h | Add recordHash to track the owning record of a function |
| plugins/cpp_metrics/model/CMakeLists.txt | Update ODB sources: remove old metrics header, add new dependency header |
| plugins/cpp/model/CMakeLists.txt | Add cpptypedependency.h to the model build |
Comments suppressed due to low confidence (3)
plugins/cpp_metrics/parser/src/cppmetricsparser.cpp:497
- After switching to
CppTypeDependency_Distinct_D_Countfor module-level efferent metrics, consider adding or updating unit tests to verify these module-level calculations.
TypeDependencyResult types = _ctx.db->query_value<model::CppTypeDependency_Distinct_D_Count>(
plugins/cpp/model/include/model/cpptypedependency.h:17
- [nitpick] The new
CppTypeDependencystruct lacks a descriptive comment explaining the purpose ofentityHashanddependencyHash. Consider adding a brief doc comment summarizing its role in dependency tracking.
struct CppTypeDependency
plugins/cpp_metrics/parser/src/cppmetricsparser.cpp:423
- [nitpick] The lambda capture list uses
[&, this], which can be simplified to[this, &]or just[&]for clarity and consistency.
util::OdbTransaction{_ctx.db}([&, this]
| #pragma db view \ | ||
| object(CppTypeDependency) \ | ||
| object(CppAstNode = EntityAstNode : CppTypeDependency::entityHash == EntityAstNode::entityHash \ | ||
| && EntityAstNode::astType == cc::model::CppAstNode::AstType::Definition) \ | ||
| object(File = EntityFile : EntityAstNode::location.file == EntityFile::id) \ | ||
| object(CppAstNode = DependencyAstNode : CppTypeDependency::dependencyHash == DependencyAstNode::entityHash \ | ||
| && DependencyAstNode::astType == cc::model::CppAstNode::AstType::Definition) \ | ||
| object(File = DependencyFile : DependencyAstNode::location.file == DependencyFile::id) |
There was a problem hiding this comment.
[nitpick] The ODB view definitions for CppTypeDependency are very repetitive across multiple structs. Consider extracting the common view clauses into a macro or helper to reduce duplication and simplify future updates.
| #pragma db view \ | |
| object(CppTypeDependency) \ | |
| object(CppAstNode = EntityAstNode : CppTypeDependency::entityHash == EntityAstNode::entityHash \ | |
| && EntityAstNode::astType == cc::model::CppAstNode::AstType::Definition) \ | |
| object(File = EntityFile : EntityAstNode::location.file == EntityFile::id) \ | |
| object(CppAstNode = DependencyAstNode : CppTypeDependency::dependencyHash == DependencyAstNode::entityHash \ | |
| && DependencyAstNode::astType == cc::model::CppAstNode::AstType::Definition) \ | |
| object(File = DependencyFile : DependencyAstNode::location.file == DependencyFile::id) | |
| #define COMMON_CPP_TYPE_DEPENDENCY_VIEW \ | |
| COMMON_CPP_TYPE_DEPENDENCY_VIEW |
In this PR, the
CppTypeDependencyMetricsparsing and various metrics calculations have been completely reworked:CppTypeDependencyMetricstable was renamed toCppTypeDependencyand is now part ofplugins/cpp/model.efferentTypeLevel.efferentTypeLevelandafferentTypeLevelmetrics calculations were reduced to a simpleDISTINCT COUNTSQL query on theCppTypeDependencytable. This significantly speeds up the metrics calculation process.efferentTypeLevelwere missing, I implemented those as wellafferentTypeLevellacked separation, those test classes were put into namespaceCC_CPP_AFFERENT_COUPLING_METRICS_TEST