Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
dongcarl opened this issue Jul 18, 2019 · 10 comments · Fixed by #29876
Closed

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

dongcarl opened this issue Jul 18, 2019 · 10 comments · Fixed by #29876
Labels
Build system

Comments

@dongcarl
Copy link
Contributor

dongcarl commented Jul 18, 2019 •
edited
Loading

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: #16344
@maflcko maflcko changed the title Check usages of #if define(...) Check usages of #if defined(...) Jul 18, 2019
@alko89
Copy link

alko89 commented Aug 4, 2019

I have gone through bitcoin_config.h and found a few of these.

@alko89 alko89 mentioned this issue Aug 5, 2019
Closed
@pierreN pierreN mentioned this issue Mar 26, 2020
Closed
@maflcko
Copy link
Member

maflcko commented May 31, 2020

Looks like #16547 (comment) is up for grabs?

@fanquake
Copy link
Member

I'm going to revive #16547 and look at turning on -Wundef.

@brokenprogrammer brokenprogrammer mentioned this issue Jun 8, 2022
Closed
@luke-jr
Copy link
Member

luke-jr commented Jun 18, 2022

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.

@hebasto
Copy link
Member

hebasto commented Sep 2, 2022

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.

@bipulkmr-crypto
Copy link

Is this issue up for grab?

@hebasto
Copy link
Member

hebasto commented Feb 12, 2023

Is this issue up for grab?

It is. See #16547.

@fanquake
Copy link
Member

It is. See #16547.

It isn't. See #25302.

@hebasto hebasto mentioned this issue Feb 23, 2023
Merged
@hebasto hebasto mentioned this issue May 18, 2023
Merged
@hebasto
Copy link
Member

hebasto commented May 18, 2023 •
edited
Loading

On the master branch @ 4e8a765 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):

  • bitcoin/configure.ac

    Line 1230 in 4e8a765

    AC_DEFINE_UNQUOTED([HAVE_FDATASYNC], [$HAVE_FDATASYNC], [Define to 1 if fdatasync is available.])

  • bitcoin/configure.ac

    Line 1245 in 4e8a765

    AC_DEFINE_UNQUOTED([HAVE_O_CLOEXEC], [$HAVE_O_CLOEXEC], [Define to 1 if O_CLOEXEC flag is available.])

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.

fanquake added a commit to bitcoin-core/gui that referenced this issue May 22, 2023
@fanquake
Merge bitcoin/bitcoin#27696: build: Do not define ENABLE_ZMQ when Z…
a106a86 
…MQ is not available

fa5831b build: Do not define `ENABLE_ZMQ` when ZMQ is not available (Hennadii Stepanov)

Pull request description:

  A new behavior is consistent with the other optional dependencies.

  The source code contains `#if ENABLE_ZMQ` lines only:
  ```
  $ git grep ENABLE_ZMQ -- src/*.cpp
  src/init.cpp:#if ENABLE_ZMQ
  src/init.cpp:#if ENABLE_ZMQ
  src/init.cpp:#if ENABLE_ZMQ
  src/init.cpp:#if ENABLE_ZMQ
  src/init.cpp:#if ENABLE_ZMQ
  ```

  Change in description line -- "Define to 1..." -->  "Define this symbol.." -- is motivated by the fact that the actual value of the defined `ENABLE_ZMQ` macro does not matter at all.

  Related to:
  - bitcoin/bitcoin#16419
  - bitcoin/bitcoin#25302

ACKs for top commit:
  TheCharlatan:
    ACK fa5831b
  jarolrod:
    ACK fa5831b

Tree-SHA512: 5e72ff0d34c4b33205338daea0aae8d7aa0e48fd633e21af01af32b7ddb0532ef68dd3dd74deb2c1d2599691929617e8c09676bcbaaf7d669b88816f866f1db2
@maflcko maflcko mentioned this issue Mar 26, 2024
Merged
@fanquake fanquake mentioned this issue Apr 15, 2024
Merged
@fanquake
Copy link
Member

Going to close this now that #29876 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

build: add -Wundef fanquake/bitcoin
7 participants
@fanquake @luke-jr @alko89 @dongcarl @maflcko @hebasto @bipulkmr-crypto