For-loops with a variable upper boundary do not differ very much from for-loops with integer literals if the value of the variable is known. Unroll these loops as well, similarly to them. This was a TODO anyway.
Alright, I stood on top of this for longer than I'd like to admit, what's happening here? Like, CondExpr, with the given testfile, should be i < n, right? Then BoundExpr is supposed to be i, and if that is equal to Matches.getNodeAs<Expr>("boundVarOperand") (which I guess is supposed to n), which I'm not sure how it could ever happen, we assign the RHS of the assignment to BoundExpr?
What does this do? Why is it needed? Which 2 test cases cover these lines?
I don't see obvious test case for which BoundNumVal would be unknown, am I wrong?
I am a bit afraid of the performance of the feature because of the extensive use of matchers. Based on my previous experiences matcher creation and disposition are expensive operations. Matchers are meant to be a kind of "static" structures created only once and destroyed at the end. Using them in the core infrastructure may negatively affect the execution time of the whole analysis. Maybe we should get rid of them before turning on this feature as default (which is the ultimate goal).
Oh, I just noticed that there was already an attempt to fix this :-) However, it seems that the review there died because the author left the work on the analyzer. My fix attempt is an answer to a user request. This patch does not intend to solve other problems, just the symbols with known value. However we are also interested in finalizing this project so it could be enabled as default in future versions of the analyzer. Maybe we should further discuss the open problems regarding it on cfe-dev or maybe in a personal meeting on Skype.
Well, there are plenty of comments I'm not sure ever got addressed. We could take an inventory of that as a start. @NoQ, do you have any *immediate* suggestion about the direction of this effort? If not, I'd be happy to dive deeper into the archaeology with Ádám and follow up with this later.
A quick archaeological dig suggests that i had no concerns with turning on unrolling by default back in 2017: http://lists.llvm.org/pipermail/cfe-dev/2017-August/055221.html - and i don't think i had any new concerns since then. I guess it'll be beneficial to make one more large-codebase run to make sure it still works, but other than that the only reason it's not enabled by default yet is because we forgot to do that :( This patch is good but not a must-have to turn loop unrolling on by default.
(note: I forgot to submit this a couple weeks ago)
LLVM is crashing on me due to the issue mentioned in D75678, but on Bitcoin, Xerces, CppCheck and Rtags I observed no difference in between the 2 runs. I recall that others mentioned that @szepet used to run his analyses with other configurations. I'll read final report and take another look later.