Szelethus wrote: Damn, look at this project, thriving as strong as ever 18 years after its conception! Great discussions!
Let me throw in my two cents. Overall, I think we should go with this patch. I agree with a great many things previously said, this one summarizes the argument for the visitor implementation quite well: >Yes, but I also wish we could achieve this without making the internals more >complicated. Having a separated component doing this has a lot of value. The question is, whether the other benefits of this approach outweigh this drawback. What I see as a particular strength here is a possibility for discarding bogus paths of execution. `BugReporterVisitor`s are retrospective tools, and can't affect the analysis as its happening. Suppressing reports later than never is still incredibly valuable, and necessary if we can't decide during analysis if subsequent reports will be bogus. A good example here is `MacroNullReturnSuppressionVisitor`, which suppresses reports that there were _directly caused by a zero/null value that originated from a macro_. If we were to prematurely sink all execution patch where a null value formed in a macro, we would flush perfectly good execution paths down the drain, leading to lower code coverage. The case is a little different here. Since we are talking about limiting how many loop iterations we want to analyze, its more a question of how many times do we want to analyze a piece of code, not if we want to analyze it at all. I mean, I can construct a made up examples where 0 or >2 iterations directly lead to code coverage loss on code that should've been analyzed, but I predict this to much more tolerable. Bugs that we detect now with little to no connection to loops in the program should be still detected, but maybe on a slightly different path of execution. Of course, this remains to be proven by actual data. Where I'm getting at with this is that if we ever decide to sink 0 or >2 iteration paths, it would be a relatively trivial change starting out from this patch. On a smaller note: I took a look at #111258 and as someone who has written his fair share of visitors, I find the code much more readable. Compared to the core, a visitor is more digestible and it is much easier to understand how it will affect the output of the analyzer. On the other hand, even though this patch complicates the core, _this is an issue stemming from the core_, so the placement of the changes seem more appropriate there. To summarize, for me, this is a deciding factor: if we only want to suppress reports for selected checkers after analysis, a visitor is the better, more readable and maintainable solution. If we want axe some paths for good, and I think we should, this patch should remain. https://github.com/llvm/llvm-project/pull/109804 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
