C++: Optimise quadratic code in PreprocessorBranchDirective

[edoardopirovano]

This change makes the quadratic code for finding the next PreprocessorBranchDirective in getNext linear by pulling out the index computation to a new predicate getIndexInBranch.

[zlaski-semmle]

I wonder if this should not be re-written using isBefore? We probably should be comparing across files in the same translation unit. I'll let others chime in as I'm also new here. :-)

[geoffw0]

Do we have any examples of projects where this code is performing poorly? It's already been through a few rounds of optimization in recent years, though I can see that if there are many #elifs on the same #if this change could be a significant improvement.

[edoardopirovano]

I have no examples of projects where this currently performs badly. However, the current performance relies on the fact that the CSE pass of the optimiser pulls out part of the code that is shared between the body of the not and the conjunction outside. A future optimiser change could break this (indeed there is one in development that would), and the change here ensures this won't be the case.

[geoffw0]

I wonder if this should not be re-written using isBefore?

We don't connect #if, #endif etc across different files, and it doesn't seem to be acceptable to compilers to do so (thankfully; an #if could then be associated with multiple #endifs and vice-versa).

Together with the fact preprocessor directives are guaranteed to be on a unique source line (I think without exception, but please correct me if I'm wrong on this) that makes getStartLine() sufficient in this case.

[geoffw0]

I just ran some tests of UselessInclude.ql (which uses getNext() via PreprocBlock.qll) on a MySQL snapshot (the slowest snapshot on https://jenkins.internal.semmle.com/job/Language-Tests/job/CPP-HeaderQueryTests/) and found either no change or perhaps a small improvement in performance. So I'm happy with performance right now.

I also prefer the new code.

CSE pass

What does CSE stand for?

[edoardopirovano]

Sorry, CSE stands for Common Subexpression Elimination. It pulls out expressions that are used in multiple places to their own predicates, in this particular case the
getIf() = other.getIf() and getLocation().getStartLine() < other.getLocation().getStartLine()
that appears both inside the not and outside it (with result instead of other). However, this shouldn't be relied on to always happen since changes to how we compile things can lead to it no longer happening.

[geoffw0]

👍