Complexity metric / checks must be performed on original source code #1547
Comments
|
@ivangalkin thx for investigations. Meanwhile I also tend to evaluate only "what you see later on in the source code view" (means without preprocessing the code).
@ALL: Where do you think do we still need preprocessed code? |
|
I dont think so, does anyone know when do we need a "full" ast?
…On Mon, 27 Aug 2018, 10:53 Günter Wirth, ***@***.***> wrote:
@ivangalkin <https://github.com/ivangalkin> thx for investigations.
Meanwhile I also tend to evaluate only "what you see later on in the source
code view" (means without preprocessing the code).
perform some set of visitors before preprocessor and some set after it
(there might be more visitors spoiled by preprocessor, e.g. the
syntax-highlighting one)
@ALL <https://github.com/ALL>: Where do you think do we still need
preprocessed code?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1547 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA_jyFzzh6WJONNnMHA6rQbf6625Io_Bks5uU6WGgaJpZM4WNRrY>
.
|
* partial solution for SonarOpenCommunity#1547 MOTIVATION: * SonarOpenCommunity#1494 made visible the fact, that preprocessor affects the complexity metric. E.g. if function uses macros, the underlying (hidden) complexity of theses macros increases the complexity of the function. This is wrong because of many reasons: 1. Cognitive complexity: Macros do not increase the cognitive complexity of the source code. To the reader they are similar to the function invocations. Example ASSERT_XX/EXPECT_XX macros of GTest. 2. Cyclomatic complexity: Similar logic might be applied to the cyclomatic complexity: macro expansions are similar to the function calls (which become inlined). In this case the cyclomatic complexity of the caller should not be increased. [Cyclomatic] complexity of the callee must be calculated separately. 3. Preprocessor doesn't distinguish between the macros, which were declared inside of the analyzed project or were included from the external header. Obviously the external source code (e.g. again very complex GTest macros) should not affect the complexity of someone's custom SonarQube project. SOLUTION: In general it's impossible to build a valid C/C++ AST without performing of preprocessor step. The original source code might be syntactically incorrect (e.g. if macros are used to "invent" some domain specific language out of C/C++). All metrics of sonar-cxx are however AST-based. (This is a very questionably decision e.g. because #if/#ifdef etc might affect the resulting complexity or [NC]LOC in dramatic way). So we cannot skip preprocessor step entirely and/or perform metrics calculation/checks on the non-preprocessed code. Sonar SSLR has a possibility to mark some tokens as generated. This property (`Token::isGeneratedCode()`) is for example used in order to ignore generated while * [NC]LOC counting (see `AbstractLineLengthCheck<>`) * CDP analysis (see `CxxCpdVisitor`) or * Highlighting (see `CxxHighlighterVisitor`) This patch ensures, that * all `AstNode`s, which were created while macro expansion are marked as generated * all complexity related visitors ignore generated `AstNode`s RESULT: PRO: 1. The complexity of "callers" is not affected by macro anymore. This fact makes the complexity check look logical again. CON: 1. the complexity of "callees" is not calculated at all. This is correct in case of external macros, but wrong w.r.t. to the project intern macros. Possible solution: preprocessor must try to extract the function-like macro declaration into some special AST-subtree. Afterwards complexity metrics/check will be able to calculate/check macros as separate language construct. By the way: it must be analyzed if and how macros affect other metrics. For example LOC etc. 2. This patch might change the complexity related metrics. Depending on how many macros were used in the analyzed project, the reduction can be radical.
|
@guwirth @guwirth I came to conclusion, that current visitors (all metric calculators and checks are AST visitors) cannot be applied to the original (non-preprocessed code). Original code might be non-parseable at all (there are far too many tricks one can make with macros and complier directives). I suggest to mark macro expansion nodes as generated and ignore them by all complexity-related visitors. Please review #1552 |
|
@ivangalkin if you test this in your own code base: how big is the difference in 'Measures/Complexity'? |
|
@guwirth we have a very GTest intensive code and the complexity decrease is about 30%
|
|
Hi All, update w.r.t. this issue: MACRO
COMPILER DIRECTIVES
#ifdef DEBUG
<150 NCLOCs>
<+50 complexity points>
#else
<250 NCLOCs>
<+150 complexity points>
#endifDepending the Meanwhile I tend to change my mind:
What do you think? Can we close the issue? |
|
Close it with #1552 |
...are calculated/performed on preprocessed code. The preprocessor falsifies the code complexity
#if#ifdef,#elseetc makes the code less complexExample of how macro expansion affects the check
FunctionComplexity:EXPECT_EQmacro adds +3 extra cyclomatic complexity points because it's expanded asswitch-case+if.Possible solutions:
Possible problems:
The text was updated successfully, but these errors were encountered: