The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Can be tested by adding a compile failure to the unreachable code. On master compilation passes, here it fails.

Then the actual code could use if constexpr(use_bsb) like a normal conditional.

This wouldn't work, because the #ifdef is still needed to "erase" the MakeSQLiteDatabase code. Recall that the header that declares MakeSQLiteDatabase isn't included when sqlite isn't selected by configure. (Same for bdb)

This wouldn't work, because the #ifdef is still needed to "erase" the MakeSQLiteDatabase code. Recall that the header that declares MakeSQLiteDatabase isn't included when sqlite isn't selected by configure. (Same for bdb)

Good point. There could be a number of ways to fix this (rearranging headers, using forward declarations), but probably not worth it

ACK fa3373d

When unreachable code isn't compiled, compile failures are not detected.

Another approach to this is to run CI with both USE_BDB defined and USE_BDB undefined.

Fix this by leaving it unreachable, but compiling it.

This produces compiler warnings about unreachable code. I guess that if we are to have deliberately unreachable code, then some pragma is needed to tell compilers not to warn about it in those particular places where it is intended.

Another approach to this is to run CI with both USE_BDB defined and USE_BDB undefined.

This is already done, but generally it is better to notify devs of compile failures immediately, instead of going through the lengthy CI pipeline.

This produces compiler warnings about unreachable code. I guess that if we are to have deliberately unreachable code, then some pragma is needed to tell compilers not to warn about it in those particular places where it is intended.

Yes, the reported issue is known and is already linked. if constexpr (true) is a standard way to achieve exactly that, without any pragmas. There is a single compiler out there that falsely prints a harmless warning. I don't understand why this has to be re-discussed in two threads over almost a month?

Anyone can revert the commit, if they think that reverting it solves a problem.