Check usages of #if defined(...) #16419

1. 
   dongcarl commented at 8:12 pm on July 18, 2019: contributor

   It would seem that in most cases, we want #if ... instead of #if defined(...), so let’s make sure that when we say #if defined(...), we actually mean it.

   Inspiration: https://github.com/bitcoin/bitcoin/pull/16344

2. dongcarl added the label Build system on Jul 18, 2019
3. dongcarl added the label good first issue on Jul 18, 2019
4. maflcko renamed this:
   Check usages of `#if define(...)`
   Check usages of `#if defined(...)`
   on Jul 18, 2019
5. 
   alko89 commented at 3:19 pm on August 4, 2019: none

   I have gone through bitcoin_config.h and found a few of these.
6. 
   maflcko commented at 1:44 pm on May 31, 2020: member

   Looks like #16547 (comment) is up for grabs?
7. 
   fanquake commented at 4:57 am on August 31, 2020: member

   I’m going to revive #16547 and look at turning on -Wundef.
8. 
   luke-jr commented at 5:03 pm on June 18, 2022: member

   I looked over the uses in 8be652e43964329c1f2dadd676916b53e5c40974 and found one bug: LevelDB is using #if defined(HAVE_O_CLOEXEC), which we are defining to 0 on systems without it - this leads to a build failure, and should be fixed.
9. 
   hebasto commented at 10:02 am on September 2, 2022: member

   I looked over the uses in 8be652e and found one bug: LevelDB is using #if defined(HAVE_O_CLOEXEC), which we are defining to 0 on systems without it - this leads to a build failure, and should be fixed.

   Fixed in #25739.

10. 
    bipulkmr-crypto commented at 9:55 am on February 11, 2023: none

    Is this issue up for grab?
11. 
    hebasto commented at 10:05 am on February 12, 2023: member

    Is this issue up for grab?

    It is. See #16547.

12. 
    fanquake commented at 1:44 pm on February 13, 2023: member

    It is. See #16547.

    It isn’t. See #25302.

13. 
    hebasto commented at 10:51 am on May 18, 2023: member

    On the master branch @ 4e8a7654f623fc0dad933b3d93dc54d8206c605d plus #27696, there are only two cases in our build system of explicit unconditional usage of the AC_DEFINE and AC_DEFINE_UNQUOTED macros (except for one which define string literals like COPYRIGHT_YEAR):

    • https://github.com/bitcoin/bitcoin/blob/4e8a7654f623fc0dad933b3d93dc54d8206c605d/configure.ac#L1230
    • https://github.com/bitcoin/bitcoin/blob/4e8a7654f623fc0dad933b3d93dc54d8206c605d/configure.ac#L1245

    That means that the HAVE_FDATASYNC and HAVE_O_CLOEXEC are always defined, and them with #if defined(...) is meaningless. All our code base uses #if ... for them.

    All other use cases of the AC_DEFINE and AC_DEFINE_UNQUOTED macros are conditional with explicitly provided value 1. That means either a macro is defined and expands to 1 or it is not defined. Therefore, both #if ... and #if defined(...) behave equally. However, we mean the latter.

    Moreover, descriptions like “Define to 1 to enable wallet functions” should be replaced with “Define this symbol to enable wallet functions” as the actual value of the defined macro does not matter at all.

    ---

    The bottom line:

    in most cases, we want #if ... instead of #if defined(...), so let’s make sure that when we say #if defined(...), we actually mean it.

    These cases are HAVE_FDATASYNC and HAVE_O_CLOEXEC only, which are OK now.

    ---

    It is worth mentioning that macros like AC_CHECK_DECLS define always define symbols implicitly.

    ---

    I don’t think any changes in the source files are required. However, in Developer Notes, we can recommend to use #if defined(...) by default except for cases when #if ... is required.

14. fanquake removed the label good first issue on May 18, 2023
15. fanquake referenced this in commit a106a86c46 on May 22, 2023
16. fanquake closed this on Jun 24, 2024

    ---

17. fanquake referenced this in commit aef5ac7f2c on Jun 24, 2024
18. 
    fanquake commented at 2:30 pm on June 24, 2024: member

    Going to close this now that #29876 is merged.

Contributors
dongcarl alko89 maflcko fanquake luke-jr hebasto bipulkmr-crypto

Labels
Build system