-
Notifications
You must be signed in to change notification settings - Fork 104
Rework type level metrics calculations #802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Rework type level metrics calculations #802
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
CppTypeDependencyMetrics
toCppTypeDependency
and update ODB views - Persist type dependencies during AST traversal in
ClangASTVisitor
and 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_Count
for 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
CppTypeDependency
struct lacks a descriptive comment explaining the purpose ofentityHash
anddependencyHash
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 |
Copilot uses AI. Check for mistakes.
In this PR, the
CppTypeDependencyMetrics
parsing and various metrics calculations have been completely reworked:CppTypeDependencyMetrics
table was renamed toCppTypeDependency
and is now part ofplugins/cpp/model
.efferentTypeLevel
.efferentTypeLevel
andafferentTypeLevel
metrics calculations were reduced to a simpleDISTINCT COUNT
SQL query on theCppTypeDependency
table. This significantly speeds up the metrics calculation process.efferentTypeLevel
were missing, I implemented those as wellafferentTypeLevel
lacked separation, those test classes were put into namespaceCC_CPP_AFFERENT_COUPLING_METRICS_TEST