-
Notifications
You must be signed in to change notification settings - Fork 105
Add option to skip C++ metrics calculations for lambdas and nested classes #800
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
Add option to skip C++ metrics calculations for lambdas and nested classes #800
Conversation
c47ea96
to
12c72cd
Compare
12c72cd
to
f1a4546
Compare
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 introduces the ability to skip certain C++ metrics (lack of cohesion, efferent/afferent coupling) for lambdas and nested classes via two new CLI options. It updates the parser to record isLambda
and declaration context
, filters queries based on the new options, surfaces the new attributes in the service output, and adds corresponding test changes.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
plugins/cpp_metrics/parser/src/cppmetricsparser.cpp | Added getCohesionRecordQuery , integrated option‐based filters, and registered two new CLI flags |
plugins/cpp_metrics/parser/include/cppmetricsparser/cppmetricsparser.h | Declared getCohesionRecordQuery and added required includes |
plugins/cpp/service/src/cppservice.cpp | Extended getProperties to include Lambda and Context fields |
plugins/cpp/parser/src/clangastvisitor.h | Populated CppRecord::isLambda and context based on parent DeclContext |
plugins/cpp/model/include/model/cpprecord.h | Added isLambda , Context enum, and getContextString() |
plugins/cpp/test/src/cpppropertiesservicetest.cpp | Updated expected properties to include Context |
Comments suppressed due to low confidence (3)
plugins/cpp/service/src/cppservice.cpp:337
- [nitpick] The property key "Lambda" is less descriptive compared to other boolean flags (e.g., "Abstract type", "POD type"). Consider renaming it to something like "Is Lambda" or "Lambda expression" for consistency and clarity.
return_["Lambda"] = "true";
plugins/cpp/test/src/cpppropertiesservicetest.cpp:83
- There are no tests verifying the new
Lambda
property whentype.isLambda
is true. Consider adding a test case that simulates a lambda record and checks for"Lambda" : "true"
in the output map.
TEST_F(CppPropertiesServiceTest, ClassPropertiesTest)
plugins/cpp_metrics/parser/src/cppmetricsparser.cpp:717
- [nitpick] The CLI option descriptions mention skipping specific metrics but could be clearer about all three metrics affected. Consider rephrasing to explicitly list or group the three metrics affected for both options.
"Skip Efferent/Afferent Coupling, Lack of Cohesion C++ metrics calculations for lambdas.")
@@ -72,6 +85,18 @@ struct CppRecord : CppEntity | |||
|
|||
return ret; | |||
} | |||
|
|||
std::string getContextString() const |
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.
The switch in getContextString()
covers all enum cases but has no default or final return, which can trigger a missing-return warning. Consider adding a default case or a return after the switch to handle unexpected values.
Copilot uses AI. Check for mistakes.
Great work! 👏 |
CppRecord
parsing: we now also store anisLambda
andcontext
attributeCppServiceHandler::getProperties
to display the new attributes--cppmetrics-ignore-lambdas
and--cppmetrics-ignore-nested-classes
to skip C++ metrics calculations in those caseslackOfCohesion
,efferentTypeLevel
,afferentTypeLevel