build: Check usages of #if defined(...)

brokenprogrammer commented at 6:39 am on June 8, 2022: contributor

I tried to go over #16419.

Based on #16344 I interpreted that the AC_DEFINE like the following:

0AC_DEFINE(USE_EXTERNAL_ASM, 1, [Define this symbol if an external (non-inline) assembly implementation is used])

Results in value defines which also #16547 suggests.

If I’ve missunderstood anything or if there is more #if defined or #ifdef that should be #if then please let me know and I will update the pull request.

fanquake added the label Build system on Jun 8, 2022

brokenprogrammer commented at 6:57 am on June 8, 2022: contributor

The linting failed because of FAIL: subtree directory was touched without subtree merge. I guess this is because I touched some external library code. Should I just revert the changes that affected secp256k1?

laanwj commented at 7:56 am on June 8, 2022: member

The linting failed because of FAIL: subtree directory was touched without subtree merge. I guess this is because I touched some external library code. Should I just revert the changes that affected secp256k1?

Yes, it’s not possible to make changes to subtrees through a PR in this repository, it needs to go through the appropriate upstream.

brokenprogrammer commented at 8:22 am on June 8, 2022: contributor

The linting failed because of FAIL: subtree directory was touched without subtree merge. I guess this is because I touched some external library code. Should I just revert the changes that affected secp256k1?

Yes, it’s not possible to make changes to subtrees through a PR in this repository, it needs to go through the appropriate upstream.

I see, thank you for the information. I have reverted the files for secp256k1.

fanquake commented at 8:31 am on June 8, 2022: member

I have reverted the files for secp256k1.

You need to remove the changes entirely, no reversion commits. You’ll also need to write a proper / useful commit message for the actual change. I’d suggest reading through https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md.

brokenprogrammer force-pushed on Jun 8, 2022

brokenprogrammer commented at 9:02 am on June 8, 2022: contributor

I have reverted the files for secp256k1.

You need to remove the changes entirely, no reversion commits. You’ll also need to write a proper / useful commit message for the actual change. I’d suggest reading through https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md.

Sorry about that, I’ve published a new change which I hope corrects my mistakes.

DrahtBot commented at 3:13 pm on June 8, 2022: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27607 (init: verify blocks data existence only once for all the indexers by furszy)
#27598 (bench: Add SHA256 implementation specific benchmarks by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

luke-jr commented at 4:55 pm on June 18, 2022: member

AC_DEFINE only defines if it’s actually called. Conditional calls to AC_DEFINE (such as those setting it to a constant 1, or not setting a value at all) need defined()/#ifdef AIUI

~~Looking through our defines, I don’t see anything that requires changes.~~ Edit: was looking at an old commit

luke-jr commented at 5:03 pm on June 18, 2022: member

Correction: 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.

brokenprogrammer commented at 9:51 am on June 19, 2022: contributor

AC_DEFINE only defines if it’s actually called. Conditional calls to AC_DEFINE (such as those setting it to a constant 1, or not setting a value at all) need defined()/#ifdef AIUI

~Looking through our defines, I don’t see anything that requires changes.~ Edit: was looking at an old commit

With that said I think I’ve missunderstood the task? Should I remove all my previous changes and perhaps only address what you mentioned with HAVE_O_CLOEXEC?

fanquake commented at 4:04 pm on August 15, 2022: member

Should I remove all my previous changes and perhaps only address what you mentioned with HAVE_O_CLOEXEC?

No, that issue has now been resolved in master. However you can rebase / squash your changes here.

fanquake commented at 10:32 am on October 5, 2022: member

@brokenprogrammer interested in following?

brokenprogrammer commented at 12:03 pm on October 5, 2022: contributor

@fanquake So the changes are fine and what is left is simply to merge latest master and squash the changes?

fanquake commented at 1:45 pm on February 13, 2023: member

@brokenprogrammer sorry for not following up. Changes should always be squashed. I will get to reviewing this shortly.

in src/bench/verify_script.cpp:7 in 6efd69abdd outdated

3@@ -4,7 +4,7 @@
4 
5 #include <bench/bench.h>
6 #include <key.h>
7-#if defined(HAVE_CONSENSUS_LIB)
8+#if HAVE_CONSENSUS_LIB

hebasto commented at 3:19 pm on February 13, 2023:

Conditional calls to AC_DEFINE (such as those setting it to a constant 1, or not setting a value at all) need defined()/#ifdef AIUI

Agree with @luke-jr.

In this particular case, it is conditional: https://github.com/bitcoin/bitcoin/blob/8126551d54ffd290ed5767248be4b3d19243787b/configure.ac#L1707-L1710

therefore, the current code (in the master branch) is fine.

build: Updated value based #ifdef to use #if d204332b11

brokenprogrammer force-pushed on Feb 13, 2023

brokenprogrammer commented at 4:20 pm on February 13, 2023: contributor

@fanquake No worries, I’ve squashed and updated the commit message according to the contributing guidelines.

hebasto commented at 9:43 am on February 14, 2023: member

@brokenprogrammer

I’ve squashed and updated the commit message according to the contributing guidelines.

A #25302 (review) still needs to be addressed.

hebasto commented at 3:42 pm on February 16, 2023: member

~Another bug found (introduced in #20358), which needs to be addressed.~

UPD. False suggestion. Sorry.

achow101 requested review from theuni on Apr 25, 2023

achow101 requested review from TheCharlatan on Apr 25, 2023

achow101 requested review from theStack on Apr 25, 2023

achow101 requested review from hebasto on Apr 25, 2023

in src/crypto/sha256.cpp:607 in d204332b11

603@@ -604,7 +604,7 @@ std::string SHA256AutoDetect()
604         have_x86_shani = (ebx >> 29) & 1;
605     }
606 
607-#if defined(ENABLE_X86_SHANI) && !defined(BUILD_BITCOIN_INTERNAL)
608+#if ENABLE_X86_SHANI && !defined(BUILD_BITCOIN_INTERNAL)

TheCharlatan commented at 8:12 pm on April 30, 2023:

I think this is only defined conditionally:

https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L569-L581

~Edit: Sorry, I think I made a mistake here.~ And another edit: I think I confused myself here. Shouldn’t any variable that is not defined in config.status with either a “0” or “1” be checked with #if defined(...)?

DrahtBot added the label Needs rebase on May 17, 2023

DrahtBot commented at 7:23 pm on May 17, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

hebasto commented at 10:54 am on May 18, 2023: member

I’ve re-reviewed this PR and none of the suggested changes seem required to me.

More details see in #16419 (comment).

fanquake commented at 11:06 am on May 18, 2023: member

Going to close for now. We can continue any discussion in #16419.

fanquake closed this on May 18, 2023

fanquake referenced this in commit a106a86c46 on May 22, 2023

bitcoin locked this on May 17, 2024

build: Check usages of #if defined(…) #25302

Reviews

Conflicts