CxxPreprocessor: remove memory bloat, delete MissingIncludeFileCheck #1638
Conversation
|
Hi @ivangalkin I’m not sure if we should remove the missing include file check. This is to give hints what’s missing in the includeDirectories. You are right that code should be always compilable and error free as a precondition. But in case you want to reduce parsing errors to create a valid AST you need includeDirectories. |
|
@guwirth in my opinion we have to differentiate between two problems: a) my application administrator is unable to setup SonarQube/sonar-cxx properly: we address this issue with tracing. If necessary we can trace each misconfigured include. However one missing include directory can cause thousands of warnings. For administrator such huge number of issues might raise his/her attention. b) my software developers create poor quality code: there are panty of useful checks which can help to avoid some hidden errors (e.g. control-flow analysis or OOP best practices, which are hard to find manually of by means of just compiling the code). Such warning are visualized in the Web-UI, they affect quality rank, they are sent via email notifications etc. Do we want, that hundreds or even thousands of "missing include issues" exposed in such way? Is it really a sign of poor software quality or just a configuration problem? How do normal developer benefits from this issues, which he/she receives daily in their mailboxes? All false-positives decrease the trust in the analysis tools. But this particular "missing include" check is the worst one. It's incredible hard to setup sonar-cxx properly. Compilation database (JSON) or MSVC log is the only reliable source of compiler configuration. But even with the compilation database I still face 13000 include issues. So if false-positives are guaranteed, why to provide such check at all? |
* remove wrong is-null checks (although I hope, that SonarOpenCommunity#1638 will be merged and we could remove the code completely)
Depends what you mean with administrator? The setup of the includes has normally to be done on scanner side and not on server side (property
That was the reason to collect warnings and display them only once.
In a first version this messages were only in the LOG file. But users were requesting for a solution they see missing includes also in the UI. It's up to the user to turn it on/off.
It's for sure no technical debt. It's a configuration problem. So question is how to handle configuration errors?
Yes and no. In worst case you have totally wrong metrics because of missing includes. Is this better? Leads to the question: How do we handle configuration errors in an unique way? Argument in the past was that most people don't look into scanner LOG file. |
|
@ivangalkin please rebase it. |
* CxxPreprocessor was misused as a global container, which
unconditionally collected
* file paths to all detected include directives
* file paths to include directives, which couldn't resolved
* MissingIncludeFileCheck was the only user of these structures
There are several reasons to remove this functionality all together
* MissingIncludeFileCheck is highly error-prone and doesn't provide any
valuable information (any compiler can do this job better than we)
* Global `HashMultimap`s prevent any attempts to make the analysis parallel
* They introduce run-time overhead and bloat the heap
ivangalkin
force-pushed
the
preprocessor_includes
branch
from
10d30af to
2d5af03
Compare
* collect missing includes only if MissingIncludeFileCheck was activated * don't collect successful includes - nobody uses them * avoid memory run-time overhead and reduce memory footprint * CxxParser::cxxpp - don't extend the lifetime of CxxPreprocessor artificially Alternative (more radical) solution: SonarOpenCommunity#1638
done I accept the argument, that some users might want to enable I pushed an alternative PR #1650, which collects missing includes, but only if corresponding check was enabled. I am ok if you prefer the alternative PR over the original one. Just a couple of words, why I don't like a) developer(s), which maintain the scanners (or rather scanners' jobs), The We already have a trace. That's the communication channel for the group a). |
|
@ivangalkin what are we doing with cxx:ParsingErrorRecovery (e.g. #1648)? Should also be in LOG file only if we follow this approach? |
|
@guwirth yes, in my opinion Imagine there would be some cxx-checks tagged as |
* remove wrong is-null checks (although I hope, that SonarOpenCommunity#1638 will be merged and we could remove the code completely)
* collect missing includes only if MissingIncludeFileCheck was activated * don't collect successful includes - nobody uses them * avoid memory run-time overhead and reduce memory footprint * CxxParser::cxxpp - don't extend the lifetime of CxxPreprocessor artificially Alternative (more radical) solution: SonarOpenCommunity#1638
CxxPreprocessor was misused as a global container, which
unconditionally collected
MissingIncludeFileCheck was the only user of these structures
There are several reasons to remove this functionality all together
valuable information (any compiler can do this job better than we)
HashMultimaps prevent any attempts to make the analysis parallelThis change is