refactor: Compile unreachable walletdb code #29315

1. 
   maflcko commented at 12:45 pm on January 25, 2024: member

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

   Fix this by leaving it unreachable, but compiling it.

   Fixes #28999 (review)

2. 
   DrahtBot commented at 12:45 pm on January 25, 2024: contributor

   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.

   Type	Reviewers
   ACK	ryanofsky, achow101

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

3. DrahtBot added the label Refactoring on Jan 25, 2024
4. 
   maflcko commented at 12:47 pm on January 25, 2024: member

   Can be tested by adding a compile failure to the unreachable code. On master compilation passes, here it fails.
5. maflcko force-pushed on Jan 25, 2024
6. refactor: Compile unreachable code

   When unreachable code isn't compiled, compile failures are not detected.
   
   Fix this by leaving it unreachable, but compiling it.
   
   Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916
   
   Can be reviewed with --ignore-all-space

   fa3373d3ad
7. maflcko force-pushed on Jan 25, 2024
8. ryanofsky approved
9. 
   ryanofsky commented at 4:52 pm on January 25, 2024: contributor

   Code review ACK fa3373d3adbace7e4665cf391363319a55a09a96. This looks good, and should prevent code in the else blocks from accidentally breaking.

   Maybe a way to make it a little clearer would be to define use_bdb and use_sqlite c++ constants like:

   0constexpr bool use_bdb{
   1#ifdef USE_BDB
   2true
   3#endif
   4};
   

   Then the actual code could use if constexpr(use_bsb) like a normal conditional. This might be more verbose than the current approach, though, and the current approach does seem fine.

10. 
    maflcko commented at 4:56 pm on January 25, 2024: member

    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)

11. 
    ryanofsky commented at 5:08 pm on January 25, 2024: contributor

    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

12. 
    achow101 commented at 10:04 pm on January 25, 2024: member

    ACK fa3373d3adbace7e4665cf391363319a55a09a96
13. achow101 merged this on Jan 25, 2024
14. achow101 closed this on Jan 25, 2024

    ---

15. maflcko deleted the branch on Jan 26, 2024
16. 
    vasild commented at 2:30 pm on February 16, 2024: contributor

    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.

17. 
    maflcko commented at 2:52 pm on February 16, 2024: member

    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.

Contributors
maflcko DrahtBot ryanofsky achow101 vasild

Labels
Refactoring