Skip to content
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

Lack of cohesion metrics #688

Merged
merged 30 commits into from
Feb 5, 2024
Merged

Lack of cohesion metrics #688

merged 30 commits into from
Feb 5, 2024

Conversation

dbukki
Copy link
Collaborator

@dbukki dbukki commented Dec 3, 2023

Introduction of Lack of Cohesion (LoC) metrics in the metrics plugin.

  • New metrics calculation function: CppMetricsParser::lackOfCohesion
  • New metrics types: LACK_OF_COHESION and LACK_OF_COHESION_HS
  • Helper function for detecting if the given file path of an AST node is internal to the parsed project: CppMetricsParser::isInInputPath
  • New helper database views in model/cppcohesionmetrics.h
  • Verbose debug utilities for the real-time monitoring of the parsed types.
  • Refactor: Removed a duplicate code block from ClangASTVisitor::getFileLoc.

Fixes #681.

@intjftw intjftw self-requested a review December 3, 2023 16:09
@intjftw intjftw added Kind: Enhancement 🌟 Plugin: C++ Issues related to the parsing and presentation of C++ projects. Plugin: Metrics Issues related to the code metrics plugin. labels Dec 3, 2023
Copy link
Collaborator

@intjftw intjftw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately this PR is fairly good. I tested it on TinyXML2, and it returns believable values. Additionally, @mcserep told me that you agreed on adding automated test cases which I don't see so please @dbukki add them along with my change requests.

plugins/cpp_metrics/parser/src/cppmetricsparser.cpp Outdated Show resolved Hide resolved
@@ -15,13 +17,27 @@

#include <memory>

// Controls whether cohesion metrics are printed to the output
// immediately as they are being calculated.
#define DEBUG_COHESION_VERBOSE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the point of this macro symbol in its current form. It's always defined, independent of the loglevel switch so the following statements between #ifdef and #endif macros are executed anyway. It would make more sense if the definition of this symbol depended on the --loglevel=debug command line argument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @intjftw , we already use Boost's log library, no need for these macros. If you wouldn't like to include all these messages to the debug log, there is even a trace log level below it, which can be used.

plugins/cpp/parser/src/clangastvisitor.h Show resolved Hide resolved
plugins/cpp_metrics/parser/src/cppmetricsparser.cpp Outdated Show resolved Hide resolved
@intjftw
Copy link
Collaborator

intjftw commented Dec 11, 2023

Something I noticed running the plugin in debug mode: it seems that one AST node is calculated multiple times:

2023-12-11 11:41:59 [DEBUG] 498/527	tinyxml2::MemPoolT::Item               0       0
2023-12-11 11:41:59 [DEBUG] 499/527	tinyxml2::MemPoolT::Block              0       0
2023-12-11 11:41:59 [DEBUG] 500/527	tinyxml2::MemPoolT                 0.625  0.6818
2023-12-11 11:41:59 [DEBUG] 501/527	tinyxml2::MemPoolT                0.7083  0.7727
2023-12-11 11:41:59 [DEBUG] 502/527	tinyxml2::MemPoolT                0.7083  0.7727
2023-12-11 11:41:59 [DEBUG] 503/527	tinyxml2::MemPoolT                0.7083  0.7727
2023-12-11 11:41:59 [DEBUG] 504/527	tinyxml2::MemPoolT                0.7083  0.7727

@dbukki is this an error or just something with the debug log?

@dbukki
Copy link
Collaborator Author

dbukki commented Dec 11, 2023

Something I noticed running the plugin in debug mode: it seems that one AST node is calculated multiple times:
...
@dbukki is this an error or just something with the debug log?

I am currently investigating this strange phenomenon too.
If the same type only ever appeared in the output with the same metrics, I would be tempted to say that this is just a printing error. But for some reason, the same type can sometimes appear with different metrics, as is the case with tinyxml2::MemPoolT in your example.

My initial guess is that these are actually different instantiations of the same templated type, and some of them are specializations that have a different number of fields and methods.
If this is indeed the case, then the fault is to be found in our already existing type extraction logic in the parser: perhaps the fully qualified type name does not yet contain the template arguments?

It could also be that types defined in a header and included by multiple compilation units each have their own copy of this type in the database.

And then it's also possible that both of the above factors play a role in this output. I have yet to figure out a full diagnosis.

Fixed the casing and naming of debug utility variables in the LoC parser.
Copy link
Collaborator

@intjftw intjftw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most critical point is the test failure in the CI. If that's corrected, I say we merge the PR, and clean up a few things afterwards. @mcserep what do you think?

docker/dev/Dockerfile Outdated Show resolved Hide resolved
plugins/cpp_metrics/test/sources/service/mccabe.cpp Outdated Show resolved Hide resolved
mcserep and others added 3 commits February 4, 2024 22:03
# Conflicts:
#	plugins/cpp_metrics/parser/include/cppmetricsparser/cppmetricsparser.h
#	plugins/cpp_metrics/parser/src/cppmetricsparser.cpp
#	util/include/util/filesystem.h
@mcserep mcserep merged commit d922b0c into Ericsson:master Feb 5, 2024
11 checks passed
@dbukki dbukki deleted the lack-of-cohesion branch February 22, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kind: Enhancement 🌟 Kind: Important 🥇 Plugin: C++ Issues related to the parsing and presentation of C++ projects. Plugin: Metrics Issues related to the code metrics plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of Cohesion of Methods at Type Level for C++
3 participants